[Top] [All Lists]

Re: [PATCH] xfsprogs: xfs_copy: use exit() to replace killall()

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfsprogs: xfs_copy: use exit() to replace killall()
From: Junxiao Bi <junxiao.bi@xxxxxxxxxx>
Date: Mon, 05 May 2014 14:13:38 +0800
Cc: xfs@xxxxxxxxxxx, joe.jin@xxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140504103443.GA25154@xxxxxxxxxxxxx>
References: <1399189455-29890-1-git-send-email-junxiao.bi@xxxxxxxxxx> <20140504103443.GA25154@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.2.0
On 05/04/2014 06:34 PM, Christoph Hellwig wrote:
> On Sun, May 04, 2014 at 03:44:15PM +0800, Junxiao Bi wrote:
>> Sending a SIGKILL signal to child thread will terminate the whole process,
>> xfs_copy will return an error value 137. This cause confuse for script to
>> know whether the copy successes.
>> Calling exit() in main thread can terminate the whole process and return the
>> right value.
> Looks generally good to me, but the changelog should have some more
> details:
Yes, will update.
>>      if (buf->length > buf->size)  {
>>              do_warn(_("assert error:  buf->length = %d, buf->size = %d\n"),
>>                      buf->length, buf->size);
>> -            killall();
>> -            abort();
>> +            exit(1);
> You're replacing the killall with an exit here and removing the abort()
> call.  I can see arguments for keeping either the abort or exit, but please
> document why you did in the patch description.  If the exit was
> intentional should we return a different value for an assertation
> failure?
I think exit() is more clear than killall()+abort(). And xfs_copy uses
exit(1) to exit before creating threads.
I think 1 is OK, since all values except 0 means xfs_copy fail.
>> -    killall();
>> -    pthread_exit(NULL);
>> -    /*NOTREACHED*/
>> -    return 0;
>> +
>> +    exit(0);
> You're also replacing the return 0 from main with an exit which
> seem superflous.
Yes, return 0 is OK. Will update the patch.
> Btw, I think the reason for this cruft is that xfs_copy was originally
> written using the IRIX sproc interface, and the port to pthreads didn't
> remove this gem:
Thanks for point the reason.

> http://marc.info/?l=linux-xfs&m=99535721110020&w=2

<Prev in Thread] Current Thread [Next in Thread>