Christoph Hellwig wrote:
On Fri, Jul 29, 2011 at 03:40:11PM -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.
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 these threads as the mask is
inherited.
From reading the code that means they actually can't be reached in
a Linux build at the moment, given that the sproc stub will always
return -1.
Right. I wanted to submit the signal changes separately from the
threading changes, as the changes were mostly independent except
in a couple of areas like this.
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.
What thread model are you going to use for the multithreaded xfsdump?
If it's pthreads the signal handlers and the main signal mask are shared
by all threads, so setting them in ring_slave_entry will affect the whole
process. We can do per-thread blocking/unblocking using pthread_sigmask,
but we can't have per-signal handlers.
Yes, it will be pthreads. My threading series converts all the sigprocmask
calls to pthread_sigmask once xfsdump links with libpthread. Should have
mentioned that in the patch description.
The original code in ring_slave_entry() changed the (process-wide) signal
dispositions. My patch converts these to just block the signals, so I
think this is fine?
I don't think you'll get around splitting drive_init1, so that we can
first open the devices, then do the is pipe check and do the signal
setup based on that, then move on to the remaining drive setup.
I thought it might be possible to avoid treating the pipeline case
separately. It's not obvious to me why xfsdump has to change its
signal handling just because it's in a pipeline. This was something
I was planning to look at.
Any chance you could throw in a patch to clean that area up a bit?
Currently ring_create gets a threadfunc argument, which has two
different but identical implementations. Moving the small content
of the two ring_thread implementations directly into ring_create
would make this a tad more readable.
Sure, I'll submit that as a separate patch.
@@ -374,13 +371,14 @@ promptinput( char *buf,
{
va_list args;
u_intgen_t alarm_save = 0;
- void (* sigalrm_save)(int) = NULL;
- void (* sigint_save)(int) = NULL;
- void (* sighup_save)(int) = NULL;
- void (* sigterm_save)(int) = NULL;
- void (* sigquit_save)(int) = NULL;
+ sigset_t dlog_set, orig_set;
+ struct sigaction sa;
+ struct sigaction sigalrm_save;
+ struct sigaction sigint_save;
+ struct sigaction sighup_save;
+ struct sigaction sigterm_save;
+ struct sigaction sigquit_save;
intgen_t nread;
- pid_t pid = getpid( );
/* display the pre-prompt
*/
@@ -400,38 +398,39 @@ promptinput( char *buf,
mlog( MLOG_NORMAL | MLOG_NOLOCK | MLOG_BARE, promptstr );
/* set up signal handling
+ * the mlog lock is held for the life of the dialog and it's possible
+ * the main thread, which normally does the signal handling, is now
+ * waiting on the mlog lock trying to log a message. so we unblock
+ * the relevant signals for this thread. note this means the current
+ * thread or the main thread might handle one of these signals.
*/
+ sigemptyset( &dlog_set );
+ sa.sa_handler = sighandler;
+ sigfillset( &sa.sa_mask );
+ sa.sa_flags = 0;
dlog_signo_received = -1;
if ( dlog_timeouts_flag && timeoutix != IXMAX ) {
+ sigaddset( &dlog_set, SIGALRM );
+ sigaction( SIGALRM, &sa, &sigalrm_save );
Why yare all these sigaction calls needed? As far as I can see
there is no way we'll ever use a different signal handler than
"sigaction" for any signal, so simply modifying the signal mask
should be enough.
There's actually 2 "sighandler" routines. One in main.c and one in
dlog.c. So this does change the handler, it's just that they're
poorly named. I'll rename the dlog version when I resubmit.
@@ -554,22 +557,32 @@ main( int argc, char *argv[] )
sigquit_received = BOOL_FALSE;
sigstray_received = BOOL_FALSE;
prbcld_cnt = 0;
+
alarm( 0 );
+
+ 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;
+ sigfillset(&sa.sa_mask);
+ sa.sa_flags = 0;
+
+ sigaction( SIGINT, &sa, NULL );
+ sigaction( SIGHUP, &sa, NULL );
+ sigaction( SIGTERM, &sa, NULL );
+ sigaction( SIGQUIT, &sa, NULL );
+ sigaction( SIGALRM, &sa, NULL );
/* 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 );
}
/* do content initialization.
@@ -588,16 +601,22 @@ main( int argc, char *argv[] )
* with just one stream.
*/
if ( miniroot || pipeline ) {
+ struct sigaction sa;
intgen_t exitcode;
- sigset( SIGINT, sighandler );
- sigset( SIGHUP, sighandler );
- sigset( SIGTERM, sighandler );
+ sa.sa_handler = sighandler;
+ sigfillset(&sa.sa_mask);
+ sa.sa_flags = 0;
+
+ sigaction( SIGINT, &sa, NULL );
+ sigaction( SIGHUP, &sa, NULL );
+ sigaction( SIGTERM, &sa, NULL );
/* 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 );
Why do we have to do this setup here again? We just did it a few
lines above, just separated by the content_init call. While the dump
content_init seems to temporarily enabled these signals, it also
seems to undo that properly. Given that structure of content_init
it's not easy to verify that it doesn't miss any, but the right fix
is to restructure that code using goto based unwinding and return
to the caller inthe state iwas left in.
Sure, will make that change.
I don't think there is a point to re-ignore SIGPIPE either way.
+ sigprocmask( SIG_SETMASK, &orig_set, NULL );
return BOOL_FALSE;
}
@@ -1782,16 +1783,12 @@ baseuuidbypass:
free( ( void * )drvpath );
}
if ( sc_inv_stmtokenp[ strmix ] == INV_TOKEN_NULL ) {
- ( void )sigrelse( SIGINT );
- ( void )sigrelse( SIGQUIT );
- ( void )sigrelse( SIGHUP );
+ sigprocmask( SIG_SETMASK, &orig_set, NULL );
return BOOL_FALSE;
As mentioned before adding an out_unmask label to this function which
restores the mask and then returns the boolean retval variable would
make the code a lot easier to audit.
Bill
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
