xfs
[Top] [All Lists]

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

To: aelder@xxxxxxx
Subject: Re: [PATCH v2 6/7] xfsdump: convert to the POSIX signal API
From: Bill Kendall <wkendall@xxxxxxx>
Date: Fri, 12 Aug 2011 14:15:58 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1313012883.2865.139.camel@doink>
References: <1312497011-24840-1-git-send-email-wkendall@xxxxxxx> <1312497011-24840-7-git-send-email-wkendall@xxxxxxx> <1313012883.2865.139.camel@doink>
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.18) Gecko/20110617 Thunderbird/3.1.11
On 08/10/2011 04:48 PM, Alex Elder wrote:
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@xxxxxxx>


Signed-off-by: Bill Kendall<wkendall@xxxxxxx>



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.

Will do.

Is there any requirement that the fields
other than sa_flags and sa_handler should
be zeroed before use?

The sa_sigaction field will only be used if sa_flags
has SA_SIGINFO set, and the sa_restored field is obsolete
and not specified by POSIX. Better to explicitly initialize
everything though, so I'll change that.


+       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.

Yes, sigaction takes a "const struct sigaction *".

Bill

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