Re: [PATCH v2 6/7] xfsdump: convert to the POSIX signal API

[Alex Elder <aelder@sgi.com>]

On Thu, 2011-08-04 at 17:30 -0500, Bill Kendall wrote:
> Convert from using the System V signal API to the POSIX API. For
> xfsdump, this mostly means replacing sigrelse/sighold with
> sigprocmask, sigset with sigaction and sigpause with sigsuspend.
> 
> Note that sigprocmask calls will be replaced with pthread_sigmask
> when pthread support is added to xfsdump.
> 
> childmain() and cldmgr_entry() are thread entry points. By the time
> they are spawned the main thread will have already set its signal
> mask, so no need to setup signals in the thread as the mask is
> inherited.
> 
> ring_slave_entry() is a thread entry point but is spawned before the
> main thread has its signal mask setup. Setup the thread's mask to
> block the same signals that the main thread will block.  The main
> thread should be reworked to set its mask earlier, but that will
> require a fair amount of refactoring that is beyond the scope of
> this patch.
> 
> Also simplify code to generate a core file to just use abort()
> rather than sending SIGQUIT and then waiting for it to arrive.

This all looks quite good to me.  I have one request
and one question below.

Reviewed-by: Alex Elder <aelder@sgi.com>


> Signed-off-by: Bill Kendall <wkendall@sgi.com>



> diff --git a/common/main.c b/common/main.c
> index 825c894..d3b6662 100644
> --- a/common/main.c
> +++ b/common/main.c
> @@ -545,13 +545,20 @@ main( int argc, char *argv[] )
>  	 * be released at pre-emption points and upon pausing in the main
>  	 * loop.
>  	 */
> +	
> +	struct sigaction sa;

Define this at the top of the block please.
Is there any requirement that the fields
other than sa_flags and sa_handler should
be zeroed before use?

> +	sigfillset(&sa.sa_mask);
> +	sa.sa_flags = 0;
>  
>  	/* always ignore SIGPIPE, instead handle EPIPE as part
>  	 * of normal sys call error handling
>  	 */
> -	sigset( SIGPIPE, SIG_IGN );
> +	sa.sa_handler = SIG_IGN;
> +	sigaction( SIGPIPE, &sa, NULL );

Are you guaranteed that the contents of "sa"
have not been modified by this call?  I'm just
asking because you reuse it below and I just
don't know whether the standard says something
about it.

>  	if ( ! miniroot && ! pipeline ) {
> +		sigset_t blocked_set;
> +
>  		stop_in_progress = BOOL_FALSE;
>  		coredump_requested = BOOL_FALSE;
>  		sighup_received = BOOL_FALSE;
> @@ -560,17 +567,23 @@ main( int argc, char *argv[] )
>  		sigquit_received = BOOL_FALSE;
>  		sigstray_received = BOOL_FALSE;
>  		prbcld_cnt = 0;
> -		sigset( SIGINT, sighandler );
> -		sighold( SIGINT );
> -		sigset( SIGHUP, sighandler );
> -		sighold( SIGHUP );
> -		sigset( SIGTERM, sighandler );
> -		sighold( SIGTERM );
> -		sigset( SIGQUIT, sighandler );
> -		sighold( SIGQUIT );
> +
>  		alarm( 0 );
> -		sigset( SIGALRM, sighandler );
> -		sighold( SIGALRM );
> +
> +		sigemptyset( &blocked_set );
> +		sigaddset( &blocked_set, SIGINT );
> +		sigaddset( &blocked_set, SIGHUP );
> +		sigaddset( &blocked_set, SIGTERM );
> +		sigaddset( &blocked_set, SIGQUIT );
> +		sigaddset( &blocked_set, SIGALRM );
> +		sigprocmask( SIG_SETMASK, &blocked_set, NULL );
> +
> +		sa.sa_handler = sighandler;
> +		sigaction( SIGINT, &sa, NULL );
> +		sigaction( SIGHUP, &sa, NULL );
> +		sigaction( SIGTERM, &sa, NULL );
> +		sigaction( SIGQUIT, &sa, NULL );
> +		sigaction( SIGALRM, &sa, NULL );
>  	}
>  
>  	/* do content initialization.

. . .

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