[PATCH 0/4] xfsdump: convert to using the POSIX signal API

[PATCH 0/4] xfsdump: convert to using the POSIX signal API

[Bill Kendall]

This patch series converts xfsdump from using the System V signal
API to using the POSIX API. The first 3 patches remove/rework some
of the existing signal code, and the final patch does the actual
conversion.

The primary motivatation for this change is a currently unused
section of code in xfsdump's main():

    /* sleep until next signal
     */
    sigrelse(SIGINT);
    sigrelse(SIGHUP);
    ...
    sigpause(SIGARLM);

The intention is to wake up if any of the signals is received, but
this will only wake up if SIGALRM is received. Using sigsuspend()
with the appropriate mask fixes the issue.

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

[PATCH 1/4] xfsdump: remove conditional OPENMASKED code

[Bill Kendall]

xfsdump contains a couple wrapper functions which block signals
during an open(2) call. This code is conditionally compiled and has
not been enabled for a long time. Remove the wrappers rather than
converting them to use the POSIX signal API.

Signed-off-by: Bill Kendall <wkendall@sgi.com>
---
 common/drive_minrmt.c   |   58 +---------------------------------------------
 common/drive_scsitape.c |   58 +---------------------------------------------
 2 files changed, 4 insertions(+), 112 deletions(-)

diff --git a/common/drive_minrmt.c b/common/drive_minrmt.c
index 94795e2..eb7876a 100644
--- a/common/drive_minrmt.c
+++ b/common/drive_minrmt.c
@@ -24,11 +24,9 @@
 #include <sys/sem.h>
 #include <sys/prctl.h>
 #include <time.h>
-#include <signal.h>
 #include <fcntl.h>
 #include <errno.h>
 #include <sys/sysmacros.h>
-#include <sys/signal.h>
 #include <malloc.h>
 #include <sched.h>
 
@@ -59,12 +57,6 @@
 
 /* remote tape protocol debug
  */
-#ifdef OPENMASKED
-static intgen_t open_masked_signals( char *path, int oflags );
-#else /* OPENMASKED */
-#define open_masked_signals(p,f)	open(p,f)
-#endif /* OPENMASKED */
-
 #ifdef RMT
 #ifdef RMTDBG
 #define	open(p,f)		dbgrmtopen(p,f)
@@ -455,7 +447,7 @@ ds_match( int argc, char *argv[], drive_t *drivep, bool_t singlethreaded )
 		    	    }
 			    cmdlineblksize = ( u_int32_t )atoi( optarg );
 		            errno = 0;
-		            fd = open_masked_signals( drivep->d_pathname, O_RDONLY );
+		            fd = open( drivep->d_pathname, O_RDONLY );
 		            if ( fd < 0 ) 
 			            return -10;
 		            close( fd );
@@ -3435,7 +3427,7 @@ Open( drive_t *drivep )
 	ASSERT( contextp->dc_fd == -1 );
 
 	errno = 0;
-	contextp->dc_fd = open_masked_signals( drivep->d_pathname, oflags );
+	contextp->dc_fd = open( drivep->d_pathname, oflags );
 
 	if ( contextp->dc_fd <= 0 ) {
 		return BOOL_FALSE;
@@ -4039,49 +4031,3 @@ isxfsdumperasetape( drive_t *drivep )
 	else 
 		return BOOL_FALSE;
 }
-
-#ifdef OPENMASKED
-
-static intgen_t
-open_masked_signals( char *pathname, int oflags )
-{
-	bool_t isrmtpr;
-	SIG_PF sigalrm_save;
-	SIG_PF sigint_save;
-	SIG_PF sighup_save;
-	SIG_PF sigquit_save;
-	SIG_PF sigcld_save;
-	intgen_t rval;
-	intgen_t saved_errno;
-
-	if ( strchr( pathname, ':') ) {
-		isrmtpr = BOOL_TRUE;
-	} else {
-		isrmtpr = BOOL_FALSE;
-	}
-
-	if ( isrmtpr ) {
-		sigalrm_save = sigset( SIGALRM, SIG_IGN );
-		sigint_save = sigset( SIGINT, SIG_IGN );
-		sighup_save = sigset( SIGHUP, SIG_IGN );
-		sigquit_save = sigset( SIGQUIT, SIG_IGN );
-		sigcld_save = sigset( SIGCLD, SIG_IGN );
-	}
-
-	errno = 0;
-	rval = open( pathname, oflags );
-	saved_errno = errno;
-
-	if ( isrmtpr ) {
-		( void )sigset( SIGCLD, sigcld_save );
-		( void )sigset( SIGQUIT, sigquit_save );
-		( void )sigset( SIGHUP, sighup_save );
-		( void )sigset( SIGINT, sigint_save );
-		( void )sigset( SIGALRM, sigalrm_save );
-	}
-
-	errno = saved_errno;
-	return rval;
-}
-
-#endif /* OPENMASKED */
diff --git a/common/drive_scsitape.c b/common/drive_scsitape.c
index 0f034b7..d72ba69 100644
--- a/common/drive_scsitape.c
+++ b/common/drive_scsitape.c
@@ -24,11 +24,9 @@
 #include <sys/sem.h>
 #include <sys/prctl.h>
 #include <time.h>
-#include <signal.h>
 #include <fcntl.h>
 #include <errno.h>
 #include <sys/sysmacros.h>
-#include <sys/signal.h>
 #include <malloc.h>
 #include <sched.h>
 
@@ -55,12 +53,6 @@
 
 /* remote tape protocol debug
  */
-#ifdef OPENMASKED
-static intgen_t open_masked_signals( char *path, int oflags );
-#else /* OPENMASKED */
-#define open_masked_signals(p,f)	open(p,f)
-#endif /* OPENMASKED */
-
 #ifdef RMT
 #ifdef RMTDBG
 #define	open(p,f)		dbgrmtopen(p,f)
@@ -539,7 +531,7 @@ ds_match( int argc, char *argv[], drive_t *drivep, bool_t singlethreaded )
 
 	if ( strchr( drivep->d_pathname, ':')) {
 		errno = 0;
-		fd = open_masked_signals( drivep->d_pathname, O_RDONLY );
+		fd = open( drivep->d_pathname, O_RDONLY );
 		if ( fd < 0 ) {
 			return -10;
 		}
@@ -4682,7 +4674,7 @@ Open( drive_t *drivep )
 	ASSERT( contextp->dc_fd == -1 );
 
 	errno = 0;
-	contextp->dc_fd = open_masked_signals( drivep->d_pathname, oflags );
+	contextp->dc_fd = open( drivep->d_pathname, oflags );
 
 	if ( contextp->dc_fd <= 0 ) {
 		return BOOL_FALSE;
@@ -5426,52 +5418,6 @@ isefsdump( drive_t *drivep )
 	}
 }
 
-#ifdef OPENMASKED
-
-static intgen_t
-open_masked_signals( char *pathname, int oflags )
-{
-	bool_t isrmtpr;
-	SIG_PF sigalrm_save;
-	SIG_PF sigint_save;
-	SIG_PF sighup_save;
-	SIG_PF sigquit_save;
-	SIG_PF sigcld_save;
-	intgen_t rval;
-	intgen_t saved_errno;
-
-	if ( strchr( pathname, ':') ) {
-		isrmtpr = BOOL_TRUE;
-	} else {
-		isrmtpr = BOOL_FALSE;
-	}
-
-	if ( isrmtpr ) {
-		sigalrm_save = sigset( SIGALRM, SIG_IGN );
-		sigint_save = sigset( SIGINT, SIG_IGN );
-		sighup_save = sigset( SIGHUP, SIG_IGN );
-		sigquit_save = sigset( SIGQUIT, SIG_IGN );
-		sigcld_save = sigset( SIGCLD, SIG_IGN );
-	}
-
-	errno = 0;
-	rval = open( pathname, oflags );
-	saved_errno = errno;
-
-	if ( isrmtpr ) {
-		( void )sigset( SIGCLD, sigcld_save );
-		( void )sigset( SIGQUIT, sigquit_save );
-		( void )sigset( SIGHUP, sighup_save );
-		( void )sigset( SIGINT, sigint_save );
-		( void )sigset( SIGALRM, sigalrm_save );
-	}
-
-	errno = saved_errno;
-	return rval;
-}
-
-#endif /* OPENMASKED */
-
 /*
  * General purpose routine which dredges through procfs trying to
  * match up device driver names with the associated major numbers
-- 
1.7.0.4

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

Re: [PATCH 1/4] xfsdump: remove conditional OPENMASKED code

[Christoph Hellwig]

On Fri, Jul 29, 2011 at 03:40:08PM -0500, Bill Kendall wrote:
> xfsdump contains a couple wrapper functions which block signals
> during an open(2) call. This code is conditionally compiled and has
> not been enabled for a long time. Remove the wrappers rather than
> converting them to use the POSIX signal API.
> 
> Signed-off-by: Bill Kendall <wkendall@sgi.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Any idea what this code was used for?  Driver-specific open quirks on
IRIX?

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

Re: [PATCH 1/4] xfsdump: remove conditional OPENMASKED code

[Bill Kendall]

Christoph Hellwig wrote:
> On Fri, Jul 29, 2011 at 03:40:08PM -0500, Bill Kendall wrote:
>> xfsdump contains a couple wrapper functions which block signals
>> during an open(2) call. This code is conditionally compiled and has
>> not been enabled for a long time. Remove the wrappers rather than
>> converting them to use the POSIX signal API.
>>
>> Signed-off-by: Bill Kendall <wkendall@sgi.com>
> 
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Any idea what this code was used for?  Driver-specific open quirks on
> IRIX?

Not sure, but probably something along those lines. The original
revision in the IRIX tree has the OPENMASKED code, but it too
is not enabled.

Thanks for the review.

Bill

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

[PATCH 2/4] xfsdump: process EPIPE instead of catching SIGPIPE

[Bill Kendall]

Looking forward towards a multi-threaded xfsdump, it's simpler to
handle pipe failures as a system call failure (EPIPE) rather than
through a signal handler which may run in a separate thread. The
existing error handling code handles EPIPE just fine, so the only
required change is to ignore SIGPIPE. Some sections of code already
temporarily ignore SIGPIPE -- they no longer need to do so since it
will already be ignored.

Signed-off-by: Bill Kendall <wkendall@sgi.com>
---
 common/main.c       |   53 +++++++++-----------------------------------------
 common/ring.c       |    1 -
 librmt/rmtcommand.c |    7 +-----
 librmt/rmtwrite.c   |    5 ----
 4 files changed, 11 insertions(+), 55 deletions(-)

diff --git a/common/main.c b/common/main.c
index c4d6878..b5a9ccf 100644
--- a/common/main.c
+++ b/common/main.c
@@ -135,7 +135,6 @@ static time32_t stop_deadline;
 static bool_t stop_in_progress;
 static bool_t sighup_received;
 static bool_t sigterm_received;
-static bool_t sigpipe_received;
 static bool_t sigquit_received;
 static bool_t sigint_received;
 static size_t prbcld_cnt;
@@ -553,7 +552,6 @@ main( int argc, char *argv[] )
 		sighup_received = BOOL_FALSE;
 		sigterm_received = BOOL_FALSE;
 		sigint_received = BOOL_FALSE;
-		sigpipe_received = BOOL_FALSE;
 		sigquit_received = BOOL_FALSE;
 		sigstray_received = BOOL_FALSE;
 		prbcld_cnt = 0;
@@ -563,8 +561,6 @@ main( int argc, char *argv[] )
 		sighold( SIGHUP );
 		sigset( SIGTERM, sighandler );
 		sighold( SIGTERM );
-		sigset( SIGPIPE, sighandler );
-		sighold( SIGPIPE );
 		sigset( SIGQUIT, sighandler );
 		sighold( SIGQUIT );
 		alarm( 0 );
@@ -572,6 +568,11 @@ main( int argc, char *argv[] )
 		sighold( SIGALRM );
 		sigset( SIGCLD, sighandler );
 		sighold( SIGCLD );
+
+		/* ignore SIGPIPE, instead handle EPIPE as part
+		 * of normal sys call error handling
+		 */
+		sigset( SIGPIPE, SIG_IGN );
 	}
 
 	/* do content initialization.
@@ -596,7 +597,11 @@ main( int argc, char *argv[] )
 		sigset( SIGINT, sighandler );
 		sigset( SIGHUP, sighandler );
 		sigset( SIGTERM, sighandler );
-		sigset( SIGPIPE, sighandler );
+
+		/* ignore SIGPIPE, instead handle EPIPE as part
+		 * of normal sys call error handling
+		 */
+		sigset( SIGPIPE, SIG_IGN );
 
 		ok = drive_init2( argc,
 				  argv,
@@ -804,16 +809,6 @@ main( int argc, char *argv[] )
 			sigterm_received = BOOL_FALSE;
 		}
 
-		/* request a stop on loss of write pipe
-		 */
-		if ( sigpipe_received ) {
-			mlog( MLOG_DEBUG | MLOG_PROC,
-			      "SIGPIPE received\n" );
-			stop_requested = BOOL_TRUE;
-			stop_timeout = STOP_TIMEOUT;
-			sigpipe_received = BOOL_FALSE;
-		}
-		
 		/* operator send SIGQUIT. treat like an interrupt,
 		 * but force a core dump
 		 */
@@ -889,14 +884,12 @@ main( int argc, char *argv[] )
 		sigrelse( SIGINT );
 		sigrelse( SIGHUP );
 		sigrelse( SIGTERM );
-		sigrelse( SIGPIPE );
 		sigrelse( SIGQUIT );
 		sigrelse( SIGALRM );
 		( void )sigpause( SIGCLD );
 		sighold( SIGCLD );
 		sighold( SIGALRM );
 		sighold( SIGQUIT );
-		sighold( SIGPIPE );
 		sighold( SIGTERM );
 		sighold( SIGHUP );
 		sighold( SIGINT );
@@ -1130,11 +1123,9 @@ preemptchk( int flg )
 	sigrelse( SIGINT );
 	sigrelse( SIGHUP );
 	sigrelse( SIGTERM );
-	sigrelse( SIGPIPE );
 	sigrelse( SIGQUIT );
 
 	sighold( SIGQUIT );
-	sighold( SIGPIPE );
 	sighold( SIGTERM );
 	sighold( SIGHUP );
 	sighold( SIGINT );
@@ -1170,13 +1161,6 @@ preemptchk( int flg )
 		sigterm_received = BOOL_FALSE;
 	}
 
-	if ( sigpipe_received ) {
-		mlog( MLOG_DEBUG | MLOG_PROC,
-		      "SIGPIPE received\n" );
-		preempt_requested = BOOL_TRUE;
-		sigpipe_received = BOOL_FALSE;
-	}
-	
 	if ( sigquit_received ) {
 		mlog( MLOG_DEBUG | MLOG_PROC,
 		      "SIGQUIT received (preempt)\n" );
@@ -1602,14 +1586,6 @@ sighandler( int signo )
 			dlog_desist( );
 			sigquit_received = BOOL_TRUE;
 			return;
-		case SIGPIPE:
-			/* immediately disable further dialogs,
-			 * and ignore subsequent signals
-			 */
-			dlog_desist( );
-			sigpipe_received = BOOL_TRUE;
-			( void )sigset( signo, SIG_IGN );
-			return;
 		case SIGALRM:
 			return;
 		default:
@@ -1638,14 +1614,6 @@ sighandler( int signo )
 			/* can get SIGQUIT during dialog: just dismiss
 			 */
 			return;
-		case SIGPIPE:
-			/* forward write pipe failures to parent,
-			 * and ignore subsequent failures
-			 */
-			dlog_desist( );
-			kill( parentpid, SIGPIPE );
-			( void )sigset( signo, SIG_IGN );
-			return;
 		case SIGALRM:
 			/* accept and do nothing about alarm signals
 			 */
@@ -1678,7 +1646,6 @@ childmain( void *arg1 )
 	sigset( SIGTERM, SIG_IGN );
 	sigset( SIGINT, SIG_IGN );
 	sigset( SIGQUIT, SIG_IGN );
-	sigset( SIGPIPE, SIG_IGN );
 	sigset( SIGALRM, SIG_IGN );
 	sigset( SIGCLD, SIG_IGN );
 
diff --git a/common/ring.c b/common/ring.c
index f6fc64d..b132ab9 100644
--- a/common/ring.c
+++ b/common/ring.c
@@ -412,7 +412,6 @@ ring_slave_entry( void *ringctxp )
 	sigset( SIGHUP, SIG_IGN );
 	sigset( SIGINT, SIG_IGN );
 	sigset( SIGQUIT, SIG_IGN );
-	sigset( SIGPIPE, SIG_IGN );
 	sigset( SIGALRM, SIG_IGN );
 	sigset( SIGCLD, SIG_IGN );
 
diff --git a/librmt/rmtcommand.c b/librmt/rmtcommand.c
index 42587e4..fbd7a6a 100644
--- a/librmt/rmtcommand.c
+++ b/librmt/rmtcommand.c
@@ -21,7 +21,6 @@
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
-#include <signal.h>
 #include <errno.h>
 
 #include "rmtlib.h"
@@ -36,19 +35,16 @@ int fildes;
 char *buf;
 {
 	register int blen;
-	void (*pstat)();
 
 	_rmt_msg(RMTDBG, "rmtcommand: fd = %d, buf = %s\n", fildes, buf);
 
 /*
- *	save current pipe status and try to make the request
+ *	try to make the request
  */
 
 	blen = strlen(buf);
-	pstat = signal(SIGPIPE, SIG_IGN);
 	if (write(WRITE(fildes), buf, blen) == blen)
 	{
-		signal(SIGPIPE, pstat);
 		return(0);
 	}
 
@@ -56,7 +52,6 @@ char *buf;
  *	something went wrong. close down and go home
  */
 
-	signal(SIGPIPE, pstat);
 	_rmt_abort(fildes);
 
 	setoserror( EIO );
diff --git a/librmt/rmtwrite.c b/librmt/rmtwrite.c
index 7f373ee..c42b1ab 100644
--- a/librmt/rmtwrite.c
+++ b/librmt/rmtwrite.c
@@ -21,7 +21,6 @@
  * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
-#include <signal.h>
 #include <errno.h>
 
 #include "rmtlib.h"
@@ -55,20 +54,16 @@ unsigned int nbyte;
 static int _rmt_write(int fildes, char *buf, unsigned int nbyte)
 {
 	char buffer[BUFMAGIC];
-	void (*pstat)();
 
 	sprintf(buffer, "W%d\n", nbyte);
 	if (_rmt_command(fildes, buffer) == -1)
 		return(-1);
 
-	pstat = signal(SIGPIPE, SIG_IGN);
 	if (write(WRITE(fildes), buf, nbyte) == nbyte)
 	{
-		signal (SIGPIPE, pstat);
 		return(_rmt_status(fildes));
 	}
 
-	signal (SIGPIPE, pstat);
 	_rmt_abort(fildes);
 	setoserror( EIO );
 	return(-1);
-- 
1.7.0.4

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

Re: [PATCH 2/4] xfsdump: process EPIPE instead of catching SIGPIPE

[Christoph Hellwig]

On Fri, Jul 29, 2011 at 03:40:09PM -0500, Bill Kendall wrote:
> Looking forward towards a multi-threaded xfsdump, it's simpler to
> handle pipe failures as a system call failure (EPIPE) rather than
> through a signal handler which may run in a separate thread. The
> existing error handling code handles EPIPE just fine, so the only
> required change is to ignore SIGPIPE. Some sections of code already
> temporarily ignore SIGPIPE -- they no longer need to do so since it
> will already be ignored.

Looks good, and the less we do with signals, the better.

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[PATCH 3/4] xfsdump: remove SIGCHLD handling

[Bill Kendall]

The multi-stream version of xfsdump for IRIX used sprocs for
threading. When a "thread" exits with sprocs, a SIGCHLD is sent to
the main thread just as if a regular child process exited. A future
multi-stream version of xfsdump would use pthreads, so the SIGCHLD
code is no longer needed. So:

- No longer register for or handle SIGCHLD (SIGCLD).
- Remove signal handling code for child processes.
- Remove cldmgr_killall() as there are no children.

Signed-off-by: Bill Kendall <wkendall@sgi.com>
---
 common/cldmgr.c |   28 ----------
 common/cldmgr.h |    4 --
 common/main.c   |  155 ++++++++++---------------------------------------------
 common/ring.c   |    1 -
 4 files changed, 28 insertions(+), 160 deletions(-)

diff --git a/common/cldmgr.c b/common/cldmgr.c
index 97507f3..3520c6e 100644
--- a/common/cldmgr.c
+++ b/common/cldmgr.c
@@ -59,16 +59,10 @@ static pid_t cldmgr_parentpid;
 bool_t
 cldmgr_init( void )
 {
-	/* REFERENCED */
-	intgen_t rval;
-
 	( void )memset( ( void * )cld, 0, sizeof( cld ));
 	cldmgr_stopflag = BOOL_FALSE;
 	cldmgr_parentpid = getpid( );
 
-	rval = atexit( cldmgr_killall );
-	ASSERT( ! rval );
-
 	return BOOL_TRUE;
 }
 
@@ -125,27 +119,6 @@ cldmgr_stop( void )
 	cldmgr_stopflag = BOOL_TRUE;
 }
 
-/* cldmgr_killall()
- *
- */
-void
-cldmgr_killall( void )
-{
-	cld_t *p = cld;
-	cld_t *ep = cld + sizeof( cld ) / sizeof( cld[ 0 ] );
-
-	signal( SIGCLD, SIG_IGN );
-	for ( ; p < ep ; p++ ) {
-		if ( p->c_busy ) {
-			mlog( MLOG_NITTY | MLOG_PROC,
-			      "sending SIGKILL to pid %d\n",
-			      p->c_pid );
-			kill( p->c_pid, SIGKILL );
-			cldmgr_died( p->c_pid );
-		}
-	}
-}
-
 void
 cldmgr_died( pid_t pid )
 {
@@ -247,7 +220,6 @@ cldmgr_entry( void *arg1 )
 	signal( SIGHUP, SIG_IGN );
 	signal( SIGINT, SIG_IGN );
 	signal( SIGQUIT, SIG_IGN );
-	signal( SIGCLD, SIG_DFL );
 	alarm( 0 );
 	cldp->c_pid = pid;
 	ok = qlock_thrdinit( );
diff --git a/common/cldmgr.h b/common/cldmgr.h
index d80fe8b..bb3f612 100644
--- a/common/cldmgr.h
+++ b/common/cldmgr.h
@@ -40,10 +40,6 @@ extern bool_t cldmgr_create( int ( * entry )( void *arg1 ),
  */
 extern void cldmgr_stop( void );
 
-/* cldmgr_killall - kills all children
- */
-extern void cldmgr_killall( void );
-
 /* cldmgr_died - tells the child manager that the child died
  */
 extern void cldmgr_died( pid_t pid );
diff --git a/common/main.c b/common/main.c
index b5a9ccf..73c63bd 100644
--- a/common/main.c
+++ b/common/main.c
@@ -483,7 +483,6 @@ main( int argc, char *argv[] )
 	 */
 	ok = drive_init1( argc, argv, miniroot );
 	if ( ! ok ) {
-		cldmgr_killall( );
 		return mlog_exit(EXIT_ERROR, RV_INIT);
 	}
 
@@ -566,8 +565,6 @@ main( int argc, char *argv[] )
 		alarm( 0 );
 		sigset( SIGALRM, sighandler );
 		sighold( SIGALRM );
-		sigset( SIGCLD, sighandler );
-		sighold( SIGCLD );
 
 		/* ignore SIGPIPE, instead handle EPIPE as part
 		 * of normal sys call error handling
@@ -584,7 +581,6 @@ main( int argc, char *argv[] )
 	ok = content_init( argc, argv, vmsz / VMSZ_PER );
 #endif /* RESTORE */
 	if ( ! ok ) {
-		cldmgr_killall( );
 		return mlog_exit(EXIT_ERROR, RV_INIT);
 	}
 
@@ -885,9 +881,7 @@ main( int argc, char *argv[] )
 		sigrelse( SIGHUP );
 		sigrelse( SIGTERM );
 		sigrelse( SIGQUIT );
-		sigrelse( SIGALRM );
-		( void )sigpause( SIGCLD );
-		sighold( SIGCLD );
+		( void )sigpause( SIGALRM );
 		sighold( SIGALRM );
 		sighold( SIGQUIT );
 		sighold( SIGTERM );
@@ -900,10 +894,6 @@ main( int argc, char *argv[] )
 	 */
 	if ( coredump_requested ) {
 		mlog( MLOG_DEBUG | MLOG_PROC,
-		      "killing all remaining children\n" );
-		cldmgr_killall( );
-		sleep( 1 );
-		mlog( MLOG_DEBUG | MLOG_PROC,
 		      "parent sending SIGQUIT to self (pid %d)\n",
 		      parentpid );
 		sigrelse( SIGQUIT );
@@ -1496,11 +1486,6 @@ mrh_sighandler( int signo )
 static void
 sighandler( int signo )
 {
-	/* get the pid and stream index
-	 */
-	pid_t pid = getpid( );
-	intgen_t stix = stream_getix( pid );
-
 	/* if in miniroot, don't do anything risky. just quit.
 	 */
 	if ( miniroot || pipeline ) {
@@ -1520,117 +1505,34 @@ sighandler( int signo )
 		exit( rval );
 	}
 
-	/* if death of a child of a child, bury the child and return.
-	 * probably rmt.
-	 */
-	if ( pid != parentpid && signo == SIGCLD ) {
-		intgen_t stat;
-		( void )wait( &stat );
-		( void )sigset( signo, sighandler );
-		return;
-	}
-
-	/* if neither parent nor managed child nor slave, exit
-	 */
-	if ( pid != parentpid && stix == -1 ) {
-		exit( 0 );
-	}
-
-	/* parent signal handling
-	 */
-	if ( pid == parentpid ) {
-		pid_t cid;
-		intgen_t stat;
-		switch ( signo ) {
-		case SIGCLD:
-			/* bury the child and notify the child manager
-			 * abstraction of its death, and record death stats
-			 */
-			cid = wait( &stat );
-			stix = stream_getix( cid );
-			cldmgr_died( cid );
-			if ( WIFSIGNALED( stat ) || WEXITSTATUS( stat ) > 0 ) {
-				if ( prbcld_cnt == 0 ) {
-					if ( WIFSIGNALED( stat )) {
-						prbcld_pid = cid;
-						prbcld_xc = 0;
-						prbcld_signo = WTERMSIG( stat );
-					} else if ( WEXITSTATUS( stat ) > 0 ) {
-						prbcld_pid = cid;
-						prbcld_xc = WEXITSTATUS( stat );
-						prbcld_signo = 0;
-					}
-				}
-				prbcld_cnt++;
-			}
-			( void )sigset( signo, sighandler );
-			return;
-		case SIGHUP:
-			/* immediately disable further dialogs
-			 */
-			dlog_desist( );
-			sighup_received = BOOL_TRUE;
-			return;
-		case SIGTERM:
-			/* immediately disable further dialogs
-			 */
-			dlog_desist( );
-			sigterm_received = BOOL_TRUE;
-			return;
-		case SIGINT:
-			sigint_received = BOOL_TRUE;
-			return;
-		case SIGQUIT:
-			/* immediately disable further dialogs
-			 */
-			dlog_desist( );
-			sigquit_received = BOOL_TRUE;
-			return;
-		case SIGALRM:
-			return;
-		default:
-			sigstray_received = signo;
-			return;
-		}
-	}
-
-	/* managed child handling
-	 */
-	if ( stream_getix( pid ) != -1 ) {
-		switch ( signo ) {
-		case SIGHUP:
-			/* can get SIGHUP during dialog: just dismiss
-			 */
-			return;
-		case SIGTERM:
-			/* can get SIGTERM during dialog: just dismiss
-			 */
-			return;
-		case SIGINT:
-			/* can get SIGINT during dialog: just dismiss
-			 */
-			return;
-		case SIGQUIT:
-			/* can get SIGQUIT during dialog: just dismiss
-			 */
-			return;
-		case SIGALRM:
-			/* accept and do nothing about alarm signals
-			 */
-			return;
-		default:
-			/* should not be any other captured signals:
-			 * request a core dump
-			 */
-			mlog_exit( EXIT_FAULT, RV_NONE );
-			exit( EXIT_FAULT );
-			return;
-		}
+	switch ( signo ) {
+	case SIGHUP:
+		/* immediately disable further dialogs
+		*/
+		dlog_desist( );
+		sighup_received = BOOL_TRUE;
+		break;
+	case SIGTERM:
+		/* immediately disable further dialogs
+		*/
+		dlog_desist( );
+		sigterm_received = BOOL_TRUE;
+		break;
+	case SIGINT:
+		sigint_received = BOOL_TRUE;
+		break;
+	case SIGQUIT:
+		/* immediately disable further dialogs
+		 */
+		dlog_desist( );
+		sigquit_received = BOOL_TRUE;
+		break;
+	case SIGALRM:
+		break;
+	default:
+		sigstray_received = signo;
+		break;
 	}
-
-	/* if some other child, just exit
-	 */
-	exit( 0 );
 }
 
 static int
@@ -1647,7 +1549,6 @@ childmain( void *arg1 )
 	sigset( SIGINT, SIG_IGN );
 	sigset( SIGQUIT, SIG_IGN );
 	sigset( SIGALRM, SIG_IGN );
-	sigset( SIGCLD, SIG_IGN );
 
 	/* Determine which stream I am.
 	 */
diff --git a/common/ring.c b/common/ring.c
index b132ab9..237d884 100644
--- a/common/ring.c
+++ b/common/ring.c
@@ -413,7 +413,6 @@ ring_slave_entry( void *ringctxp )
 	sigset( SIGINT, SIG_IGN );
 	sigset( SIGQUIT, SIG_IGN );
 	sigset( SIGALRM, SIG_IGN );
-	sigset( SIGCLD, SIG_IGN );
 
 	/* record slave pid to be used to kill slave
 	 */
-- 
1.7.0.4

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

Re: [PATCH 3/4] xfsdump: remove SIGCHLD handling

[Christoph Hellwig]

On Fri, Jul 29, 2011 at 03:40:10PM -0500, Bill Kendall wrote:
> The multi-stream version of xfsdump for IRIX used sprocs for
> threading. When a "thread" exits with sprocs, a SIGCHLD is sent to
> the main thread just as if a regular child process exited. A future
> multi-stream version of xfsdump would use pthreads, so the SIGCHLD
> code is no longer needed. So:
> 
> - No longer register for or handle SIGCHLD (SIGCLD).
> - Remove signal handling code for child processes.
> - Remove cldmgr_killall() as there are no children.
> 
> Signed-off-by: Bill Kendall <wkendall@sgi.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

does this mean you're actively working on multi-threaded dump for Linux?

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

Re: [PATCH 3/4] xfsdump: remove SIGCHLD handling

[Bill Kendall]

Christoph Hellwig wrote:
> On Fri, Jul 29, 2011 at 03:40:10PM -0500, Bill Kendall wrote:
>> The multi-stream version of xfsdump for IRIX used sprocs for
>> threading. When a "thread" exits with sprocs, a SIGCHLD is sent to
>> the main thread just as if a regular child process exited. A future
>> multi-stream version of xfsdump would use pthreads, so the SIGCHLD
>> code is no longer needed. So:
>>
>> - No longer register for or handle SIGCHLD (SIGCLD).
>> - Remove signal handling code for child processes.
>> - Remove cldmgr_killall() as there are no children.
>>
>> Signed-off-by: Bill Kendall <wkendall@sgi.com>
> 
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> does this mean you're actively working on multi-threaded dump for Linux?

Indeed...I didn't want to come right out and say it, in case I get side
tracked. :) But hoping to have patches up for review in the next week or
two.

Bill

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

[PATCH 4/4] xfsdump: convert to the POSIX signal API

[Bill Kendall]

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.

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 just use abort() to generate a core file rather than sending
SIGQUIT and then waiting for it to arrive.

Signed-off-by: Bill Kendall <wkendall@sgi.com>
---
 common/cldmgr.c |    5 ---
 common/dlog.c   |   84 +++++++++++++++++++---------------------------
 common/main.c   |  100 ++++++++++++++++++++++++++++---------------------------
 common/ring.c   |   14 +++++---
 dump/content.c  |   21 +++++-------
 5 files changed, 104 insertions(+), 120 deletions(-)

diff --git a/common/cldmgr.c b/common/cldmgr.c
index 3520c6e..7784a15 100644
--- a/common/cldmgr.c
+++ b/common/cldmgr.c
@@ -23,7 +23,6 @@
 #include <sys/ipc.h>
 #include <sys/sem.h>
 #include <sys/prctl.h>
-#include <signal.h>
 #include <errno.h>
 
 #include "types.h"
@@ -217,10 +216,6 @@ cldmgr_entry( void *arg1 )
 	/* REFERENCED */
 	bool_t ok;
 
-	signal( SIGHUP, SIG_IGN );
-	signal( SIGINT, SIG_IGN );
-	signal( SIGQUIT, SIG_IGN );
-	alarm( 0 );
 	cldp->c_pid = pid;
 	ok = qlock_thrdinit( );
 	ASSERT( ok );
diff --git a/common/dlog.c b/common/dlog.c
index 6a243ef..d37bf1c 100644
--- a/common/dlog.c
+++ b/common/dlog.c
@@ -30,9 +30,6 @@
 #include "dlog.h"
 #include "getopt.h"
 
-extern bool_t miniroot;
-extern pid_t parentpid;
-
 static int dlog_ttyfd = -1;
 static bool_t dlog_allowed_flag = BOOL_FALSE;
 static bool_t dlog_timeouts_flag = BOOL_FALSE;
@@ -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 ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sigrelse( SIGALRM );
-		}
-		sigalrm_save = sigset( SIGALRM, sighandler );
+		sigaddset( &dlog_set, SIGALRM );
+		sigaction( SIGALRM, &sa, &sigalrm_save );
 		alarm_save = alarm( ( u_intgen_t )timeout );
 	}
 	if ( sigintix != IXMAX ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sigrelse( SIGINT );
-		}
-		sigint_save = sigset( SIGINT, sighandler );
+		sigaddset( &dlog_set, SIGINT );
+		sigaction( SIGINT, &sa, &sigint_save );
 	}
 	if ( sighupix != IXMAX ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sigrelse( SIGHUP );
-		}
-		sighup_save = sigset( SIGHUP, sighandler );
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sigrelse( SIGTERM );
-		}
-		sigterm_save = sigset( SIGTERM, sighandler );
+		sigaddset( &dlog_set, SIGHUP );
+		sigaction( SIGHUP, &sa, &sighup_save );
+		sigaddset( &dlog_set, SIGTERM );
+		sigaction( SIGTERM, &sa, &sigterm_save );
 	}
 	if ( sigquitix != IXMAX ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sigrelse( SIGQUIT );
-		}
-		sigquit_save = sigset( SIGQUIT, sighandler );
+		sigaddset( &dlog_set, SIGQUIT );
+		sigaction( SIGQUIT, &sa, &sigquit_save );
 	}
 
+	sigprocmask( SIG_UNBLOCK, &dlog_set, &orig_set );
+
 	/* wait for input, timeout, or interrupt
 	 */
 	ASSERT( dlog_ttyfd >= 0 );
@@ -439,34 +438,21 @@ promptinput( char *buf,
 
 	/* restore signal handling
 	 */
+	sigprocmask( SIG_SETMASK, &orig_set, NULL );
+
 	if ( sigquitix != IXMAX ) {
-		( void )sigset( SIGQUIT, sigquit_save );
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sighold( SIGQUIT );
-		}
+		sigaction( SIGQUIT, &sigquit_save, NULL );
 	}
 	if ( sighupix != IXMAX ) {
-		( void )sigset( SIGHUP, sighup_save );
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sighold( SIGHUP );
-		}
-		( void )sigset( SIGTERM, sigterm_save );
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sighold( SIGTERM );
-		}
+		sigaction( SIGHUP, &sighup_save, NULL );
+		sigaction( SIGTERM, &sigterm_save, NULL );
 	}
 	if ( sigintix != IXMAX ) {
-		( void )sigset( SIGINT, sigint_save );
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sighold( SIGINT );
-		}
+		sigaction( SIGINT, &sigint_save, NULL );
 	}
 	if ( dlog_timeouts_flag && timeoutix != IXMAX ) {
 		( void )alarm( alarm_save );
-		( void )sigset( SIGALRM, sigalrm_save );
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sighold( SIGALRM );
-		}
+		sigaction( SIGALRM, &sigalrm_save, NULL );
 	}
 	
 	/* check for timeout or interrupt
diff --git a/common/main.c b/common/main.c
index 73c63bd..8c2c66d 100644
--- a/common/main.c
+++ b/common/main.c
@@ -546,6 +546,9 @@ main( int argc, char *argv[] )
 	 * loop.
 	 */
 	if ( ! miniroot && ! pipeline ) {
+		struct sigaction sa;
+		sigset_t blocked_set;
+
 		stop_in_progress = BOOL_FALSE;
 		coredump_requested = BOOL_FALSE;
 		sighup_received = BOOL_FALSE;
@@ -554,22 +557,32 @@ 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;
+		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 );
 
 		ok = drive_init2( argc,
 				  argv,
@@ -689,6 +708,7 @@ main( int argc, char *argv[] )
 		time32_t now;
 		bool_t stop_requested = BOOL_FALSE;
 		intgen_t stop_timeout = -1;
+		sigset_t empty_set;
 
 		/* if there was an initialization error,
 		 * immediately stop all children.
@@ -877,16 +897,8 @@ main( int argc, char *argv[] )
 
 		/* sleep until next signal
 		 */
-		sigrelse( SIGINT );
-		sigrelse( SIGHUP );
-		sigrelse( SIGTERM );
-		sigrelse( SIGQUIT );
-		( void )sigpause( SIGALRM );
-		sighold( SIGALRM );
-		sighold( SIGQUIT );
-		sighold( SIGTERM );
-		sighold( SIGHUP );
-		sighold( SIGINT );
+		sigemptyset( &empty_set );
+		sigsuspend( &empty_set );
 		( void )alarm( 0 );
 	}
 
@@ -894,14 +906,9 @@ main( int argc, char *argv[] )
 	 */
 	if ( coredump_requested ) {
 		mlog( MLOG_DEBUG | MLOG_PROC,
-		      "parent sending SIGQUIT to self (pid %d)\n",
+		      "core dump requested, aborting (pid %d)\n",
 		      parentpid );
-		sigrelse( SIGQUIT );
-		sigset( SIGQUIT, SIG_DFL );
-		kill( parentpid, SIGQUIT );
-		for ( ; ; ) {
-			sleep( 1 );
-		}
+		abort();
 	}
 
 	/* determine if dump or restore was interrupted
@@ -1074,6 +1081,10 @@ bool_t
 preemptchk( int flg )
 {
 	bool_t preempt_requested;
+	int i;
+	int sigs[] = { SIGINT, SIGHUP, SIGTERM, SIGQUIT };
+	int num_sigs = sizeof(sigs) / sizeof(sigs[0]);
+	sigset_t pending_set, handle_set;
 
 	/* see if a progress report needed
 	 */
@@ -1110,15 +1121,14 @@ preemptchk( int flg )
 	/* release signals momentarily to let any pending ones
 	 * invoke signal handler and set flags
 	 */
-	sigrelse( SIGINT );
-	sigrelse( SIGHUP );
-	sigrelse( SIGTERM );
-	sigrelse( SIGQUIT );
-
-	sighold( SIGQUIT );
-	sighold( SIGTERM );
-	sighold( SIGHUP );
-	sighold( SIGINT );
+	sigpending( &pending_set );
+	for ( i = 0; i < num_sigs; i++ ) {
+		if ( sigismember( &pending_set, sigs[i] ) == 1 ) {
+			sigfillset( &handle_set );
+			sigdelset( &handle_set, sigs[i] );
+			sigsuspend( &handle_set );
+		}
+	}
 
 	preempt_requested = BOOL_FALSE;
 
@@ -1542,14 +1552,6 @@ childmain( void *arg1 )
 	intgen_t exitcode;
 	drive_t *drivep;
 
-	/* ignore signals
-	 */
-	sigset( SIGHUP, SIG_IGN );
-	sigset( SIGTERM, SIG_IGN );
-	sigset( SIGINT, SIG_IGN );
-	sigset( SIGQUIT, SIG_IGN );
-	sigset( SIGALRM, SIG_IGN );
-
 	/* Determine which stream I am.
 	 */
 	stix = ( ix_t )arg1;
diff --git a/common/ring.c b/common/ring.c
index 237d884..f6e95e8 100644
--- a/common/ring.c
+++ b/common/ring.c
@@ -404,15 +404,19 @@ ring_slave_put( ring_t *ringp, ring_msg_t *msgp )
 static int
 ring_slave_entry( void *ringctxp )
 {
+	sigset_t blocked_set;
 	ring_t *ringp = ( ring_t * )ringctxp;
 	enum { LOOPMODE_NORMAL, LOOPMODE_IGNORE, LOOPMODE_DIE } loopmode;
 
-	/* ignore signals
+	/* block signals, let the main thread handle them
 	 */
-	sigset( SIGHUP, SIG_IGN );
-	sigset( SIGINT, SIG_IGN );
-	sigset( SIGQUIT, SIG_IGN );
-	sigset( SIGALRM, SIG_IGN );
+	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 );
 
 	/* record slave pid to be used to kill slave
 	 */
diff --git a/dump/content.c b/dump/content.c
index c6840e5..fcc952e 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -1701,6 +1701,7 @@ baseuuidbypass:
 	if ( sc_inv_updatepr ) {
 		char *qmntpnt;
 		char *qfsdevice;
+		sigset_t tty_set, orig_set;
 
 		rval = atexit( inv_cleanup );
 		ASSERT( ! rval );
@@ -1738,9 +1739,11 @@ baseuuidbypass:
 
 		/* hold tty signals while creating a new inventory session
 		 */
-		( void )sighold( SIGINT );
-		( void )sighold( SIGQUIT );
-		( void )sighold( SIGHUP );
+		sigemptyset( &tty_set );
+		sigaddset( &tty_set, SIGINT );
+		sigaddset( &tty_set, SIGQUIT );
+		sigaddset( &tty_set, SIGHUP );
+		sigprocmask( SIG_BLOCK, &tty_set, &orig_set );
 
 		sc_inv_sestoken = inv_writesession_open( sc_inv_idbtoken,
 						&fsid,
@@ -1755,9 +1758,7 @@ baseuuidbypass:
 						qmntpnt,
 						qfsdevice );
 		if( sc_inv_sestoken == INV_TOKEN_NULL ) {
-			( void )sigrelse( SIGINT );
-			( void )sigrelse( SIGQUIT );
-			( void )sigrelse( SIGHUP );
+			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;
 			}
 		}
 
-		( void )sigrelse( SIGINT );
-		( void )sigrelse( SIGQUIT );
-		( void )sigrelse( SIGHUP );
+		sigprocmask( SIG_SETMASK, &orig_set, NULL );
 	}
 
 	/* set media change flags to FALSE;
-- 
1.7.0.4

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

Re: [PATCH 4/4] xfsdump: convert to the POSIX signal API

[Christoph Hellwig]

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.

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

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.

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.

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

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

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.

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

Re: [PATCH 4/4] xfsdump: convert to the POSIX signal API

[Christoph Hellwig]

On Wed, Aug 03, 2011 at 06:48:13AM -0400, Christoph Hellwig wrote:
> 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.

Sorry, the on eand only signal handler is called "sighandler" of course.

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

Re: [PATCH 4/4] xfsdump: convert to the POSIX signal API

[Bill Kendall]

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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/4] xfsdump: convert to the POSIX signal API

[Christoph Hellwig]

On Wed, Aug 03, 2011 at 07:11:15AM -0500, Bill Kendall wrote:
> 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.

Sounds fine - it's just something I noticed when looking over the code.

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

It probably is fine, but still not very nice code.  Let's hope we
can get this sorted out.

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

Good to know.  Maybe the ptools history is able to explain something
about it?

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

You're right.  But looking at the dlog handler I still don't
like the code there very much.  It just saves the caught signal
in a global variable.  What if we got more than one signal
in that section?    Moreover I don't really understand the point
of the code at all, maybe the lack of existance of the mlog lock
referred to in your comment in the current is part of that.

I suspect the problem is that the normal signal handler may acquite
that lock?  Given that pthread locking primitives are not allowed
to be called from signal handlers that means there is a bigger
problem to solve.

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

Re: [PATCH 4/4] xfsdump: convert to the POSIX signal API

[Bill Kendall]

Christoph Hellwig wrote:
> On Wed, Aug 03, 2011 at 07:11:15AM -0500, Bill Kendall wrote:
>> 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.
> 
> Sounds fine - it's just something I noticed when looking over the code.
> 
>> 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?
> 
> It probably is fine, but still not very nice code.  Let's hope we
> can get this sorted out.
> 
>>> 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.
> 
> Good to know.  Maybe the ptools history is able to explain something
> about it?

The original code never blocked signals in 'miniroot', then later code
was added to do the same if in a pipeline. The commit message is of
no help, but reading between the lines I'd say that since a pipeline
implies a single stream, they decided to lump that in with the
single-thread (miniroot) case. Maybe I'm just seeing what I want to
see though. :)

> 
>>> 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.
> 
> You're right.  But looking at the dlog handler I still don't
> like the code there very much.  It just saves the caught signal
> in a global variable.  What if we got more than one signal
> in that section?    Moreover I don't really understand the point
> of the code at all, maybe the lack of existance of the mlog lock
> referred to in your comment in the current is part of that.
> 
> I suspect the problem is that the normal signal handler may acquite
> that lock?  Given that pthread locking primitives are not allowed
> to be called from signal handlers that means there is a bigger
> problem to solve.

Here's some background explaining why things are done as they
are now, from my understanding of the code.

The regular handler won't acquire a lock. The signal handler is
replaced because the rules are different when receiving a signal
while in a dialog. For instance, SIGINT normally means interrupt
the dump session, but in a dialog we just return a caller-supplied
value indicating the interrupt.

When a dialog is required, the caller does this:

    dlog_begin();   // grabs mlog_lock
    dlog_*_query(); // ends up in promptinput()
    dlog_end();     // releases mlog_lock

I think the purpose of holding the lock is simply to prevent
other output on the terminal while waiting for a response.

Any thread may issue a dialog, and it's possible that while
a thread is sitting in a dialog, the main thread may try to
log a message (e.g., progress report) and get blocked on the
mlog lock. At this point nobody would be able to handle signals --
the main thread blocks all signals except while in sigsuspend,
and other threads always block signals. So we unblock the
signals in the current thread to ensure some thread is available
to handle them.

I do have another patch in progress for this area of the code
to remove the use of alarm() for the timeout, as alarm() is
already used in the main thread. It also addresses an issue in
the existing code which relies on the current thread receiving
the signal. If another thread receives it, we won't come out of
the read(). If you have suggestions on others ways to clean
this up, I'd be happy to hear them.

Thanks,
Bill

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

Re: [PATCH 4/4] xfsdump: convert to the POSIX signal API

[Christoph Hellwig]

On Wed, Aug 03, 2011 at 02:28:54PM -0500, Bill Kendall wrote:
> Here's some background explaining why things are done as they
> are now, from my understanding of the code.
> 
> The regular handler won't acquire a lock. The signal handler is
> replaced because the rules are different when receiving a signal
> while in a dialog. For instance, SIGINT normally means interrupt
> the dump session, but in a dialog we just return a caller-supplied
> value indicating the interrupt.
> 
> When a dialog is required, the caller does this:
> 
>    dlog_begin();   // grabs mlog_lock
>    dlog_*_query(); // ends up in promptinput()
>    dlog_end();     // releases mlog_lock
> 
> I think the purpose of holding the lock is simply to prevent
> other output on the terminal while waiting for a response.

Ok, that makes some sense.

> Any thread may issue a dialog, and it's possible that while
> a thread is sitting in a dialog, the main thread may try to
> log a message (e.g., progress report) and get blocked on the
> mlog lock. At this point nobody would be able to handle signals --
> the main thread blocks all signals except while in sigsuspend,
> and other threads always block signals. So we unblock the
> signals in the current thread to ensure some thread is available
> to handle them.

Unblocking the signals during the dialog, but still using the normal
signal handler for it would solve that problem, right?

Btw, I looked over the main sighandler a bit, and it seems like most
of it can simply go away for a pthreaded variant - there is no need
to handle SIGCLD, and all threads have the same pid, so basically
what is left is SIGHUP/SIGTERM/SIGINT/SIGQUIT handling, which does
nothing but a dlog_desist in most cases and setting the sigfoo_received
variable.

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

Re: [PATCH 4/4] xfsdump: convert to the POSIX signal API

[Bill Kendall]

On 08/04/2011 02:53 AM, Christoph Hellwig wrote:
> On Wed, Aug 03, 2011 at 02:28:54PM -0500, Bill Kendall wrote:
>> Here's some background explaining why things are done as they
>> are now, from my understanding of the code.
>>
>> The regular handler won't acquire a lock. The signal handler is
>> replaced because the rules are different when receiving a signal
>> while in a dialog. For instance, SIGINT normally means interrupt
>> the dump session, but in a dialog we just return a caller-supplied
>> value indicating the interrupt.
>>
>> When a dialog is required, the caller does this:
>>
>>     dlog_begin();   // grabs mlog_lock
>>     dlog_*_query(); // ends up in promptinput()
>>     dlog_end();     // releases mlog_lock
>>
>> I think the purpose of holding the lock is simply to prevent
>> other output on the terminal while waiting for a response.
>
> Ok, that makes some sense.
>
>> Any thread may issue a dialog, and it's possible that while
>> a thread is sitting in a dialog, the main thread may try to
>> log a message (e.g., progress report) and get blocked on the
>> mlog lock. At this point nobody would be able to handle signals --
>> the main thread blocks all signals except while in sigsuspend,
>> and other threads always block signals. So we unblock the
>> signals in the current thread to ensure some thread is available
>> to handle them.
>
> Unblocking the signals during the dialog, but still using the normal
> signal handler for it would solve that problem, right?

Right, with some rework of that handler. It would have to do
something like:

   case SIGINT:
       if (is_dialog_active(SIGINT))
           dlg_sigterm_received = BOOL_TRUE;
       else
           sigterm_received = BOOL_TRUE;

(The SIGINT param is needed because it's optional whether a
dialog handles a particular signal.)

Otherwise we'd race between main's use of sigterm_received and
the dialog's need to use it.

Do you prefer this over the signal handler swap?

>
> Btw, I looked over the main sighandler a bit, and it seems like most
> of it can simply go away for a pthreaded variant - there is no need
> to handle SIGCLD, and all threads have the same pid, so basically
> what is left is SIGHUP/SIGTERM/SIGINT/SIGQUIT handling, which does
> nothing but a dlog_desist in most cases and setting the sigfoo_received
> variable.

Yes, the previous patch in this series takes care of that. :)

Bill

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

Re: [PATCH 4/4] xfsdump: convert to the POSIX signal API

[Christoph Hellwig]

On Thu, Aug 04, 2011 at 07:35:04AM -0500, Bill Kendall wrote:
> Right, with some rework of that handler. It would have to do
> something like:
> 
>   case SIGINT:
>       if (is_dialog_active(SIGINT))
>           dlg_sigterm_received = BOOL_TRUE;
>       else
>           sigterm_received = BOOL_TRUE;
> 
> (The SIGINT param is needed because it's optional whether a
> dialog handles a particular signal.)
> 
> Otherwise we'd race between main's use of sigterm_received and
> the dialog's need to use it.
> 
> Do you prefer this over the signal handler swap?

This seems cleaner to me.  The upside is that we a) don't have
to mess with changing signal handlers all the time, and b)
that we don't really have to bother who is going to receive the
signal.

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

Re: [PATCH 0/4] xfsdump: convert to using the POSIX signal API

[Christoph Hellwig]

Bill,

one thing I noticed looking over the code to understand your changes
is that there still is a lot of dead code in xfsdump.  One that
has a lot of implications and seems fairly useless for any non-IRIX
version is the miniroot mode.  Any chance we can rip that out?  There's
also a lot of code under #ifdef HIDDEN - it would be good to go through
these and see we can just be removed, and if anything is still useful
moved under more self-explaining ifdefs.

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

Re: [PATCH 0/4] xfsdump: convert to using the POSIX signal API

[Bill Kendall]

Christoph Hellwig wrote:
> Bill,
> 
> one thing I noticed looking over the code to understand your changes
> is that there still is a lot of dead code in xfsdump.  One that
> has a lot of implications and seems fairly useless for any non-IRIX
> version is the miniroot mode.  Any chance we can rip that out?  There's

Yes, that's being used to disable multi-stream code in the Linux version.
The end of my multi-stream patch series removes the miniroot code.

> also a lot of code under #ifdef HIDDEN - it would be good to go through
> these and see we can just be removed, and if anything is still useful
> moved under more self-explaining ifdefs.

Will add it to my list.

Bill

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

Re: [PATCH 0/4] xfsdump: convert to using the POSIX signal API

[Christoph Hellwig]

On Wed, Aug 03, 2011 at 06:57:37AM -0500, Bill Kendall wrote:
> Christoph Hellwig wrote:
> >Bill,
> >
> >one thing I noticed looking over the code to understand your changes
> >is that there still is a lot of dead code in xfsdump.  One that
> >has a lot of implications and seems fairly useless for any non-IRIX
> >version is the miniroot mode.  Any chance we can rip that out?  There's
> 
> Yes, that's being used to disable multi-stream code in the Linux version.
> The end of my multi-stream patch series removes the miniroot code.

Ooops, I didn't notice miniroot is actually defined to true.  Now that
I look deeper that also answers my comment about the duplicate signal
setup in main - one it's done for the miniroot case, and once for the
!miniroot case.

Now that we uses EPIPE instead of SIGPIPE is there actually any good
reason to do this setup in two different places?

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

Re: [PATCH 0/4] xfsdump: convert to using the POSIX signal API

[Bill Kendall]

Christoph Hellwig wrote:
> On Wed, Aug 03, 2011 at 06:57:37AM -0500, Bill Kendall wrote:
>> Christoph Hellwig wrote:
>>> Bill,
>>>
>>> one thing I noticed looking over the code to understand your changes
>>> is that there still is a lot of dead code in xfsdump.  One that
>>> has a lot of implications and seems fairly useless for any non-IRIX
>>> version is the miniroot mode.  Any chance we can rip that out?  There's
>> Yes, that's being used to disable multi-stream code in the Linux version.
>> The end of my multi-stream patch series removes the miniroot code.
> 
> Ooops, I didn't notice miniroot is actually defined to true.  Now that
> I look deeper that also answers my comment about the duplicate signal
> setup in main - one it's done for the miniroot case, and once for the
> !miniroot case.
> 
> Now that we uses EPIPE instead of SIGPIPE is there actually any good
> reason to do this setup in two different places?

Nope, I'll resubmit with this change.

Bill

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