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

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

[Junxiao Bi]

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.

Cc: Joe Jin <joe.jin@oracle.com>
Signed-off-by: Junxiao Bi <junxiao.bi@oracle.com>
---
 copy/xfs_copy.c |   33 +++------------------------------
 1 files changed, 3 insertions(+), 30 deletions(-)

diff --git a/copy/xfs_copy.c b/copy/xfs_copy.c
index 39517da..1419ce7 100644
--- a/copy/xfs_copy.c
+++ b/copy/xfs_copy.c
@@ -217,25 +217,6 @@ handle_error:
 }
 
 void
-killall(void)
-{
-	int i;
-
-	/* only the parent gets to kill things */
-
-	if (getpid() != parent_pid)
-		return;
-
-	for (i = 0; i < num_targets; i++)  {
-		if (target[i].state == ACTIVE)  {
-			/* kill up target threads */
-			pthread_kill(target[i].pid, SIGKILL);
-			pthread_mutex_unlock(&targ[i].wait);
-		}
-	}
-}
-
-void
 handler(int sig)
 {
 	pid_t	pid = getpid();
@@ -400,8 +381,7 @@ read_wbuf(int fd, wbuf *buf, xfs_mount_t *mp)
 	if (buf->length > buf->size)  {
 		do_warn(_("assert error:  buf->length = %d, buf->size = %d\n"),
 			buf->length, buf->size);
-		killall();
-		abort();
+		exit(1);
 	}
 
 	if ((res = read(fd, buf->data, buf->length)) < 0)  {
@@ -591,11 +571,6 @@ main(int argc, char **argv)
 
 	parent_pid = getpid();
 
-	if (atexit(killall))  {
-		do_log(_("%s: couldn't register atexit function.\n"), progname);
-		die_perror();
-	}
-
 	/* open up source -- is it a file? */
 
 	open_flags = O_RDONLY;
@@ -1154,10 +1129,8 @@ main(int argc, char **argv)
 	}
 
 	check_errors();
-	killall();
-	pthread_exit(NULL);
-	/*NOTREACHED*/
-	return 0;
+
+	exit(0);
 }
 
 xfs_caddr_t
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Christoph Hellwig]

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:

>  	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?

> -	killall();
> -	pthread_exit(NULL);
> -	/*NOTREACHED*/
> -	return 0;
> +
> +	exit(0);

You're also replacing the return 0 from main with an exit which
seem superflous.

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:

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Junxiao Bi]

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.

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs