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

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

[Bill Kendall]

Changes from v1:

[v2 1/7] xfsdump: remove conditional OPENMASKED code
    - No changes since v1.

[v2 2/7] xfsdump: process EPIPE instead of catching SIGPIPE
    - Minor change to ignore SIGPIPE in one location rather
      than separately for the miniroot and !miniroot cases.

[v2 3/7] xfsdump: remove SIGCHLD handling
    - No changes since v1.

[v2 4/7] xfsdump: rework dialog timeout and EINTR reliance
    - New patch to cleanup signal handling in dialog code.
      No longer use alarm() and don't depend on a particular
      thread receiving a signal to break out of a blocking
      read().

[v2 5/7] xfsdump: rework dialog to use main signal handler
    - New patch to avoid swapping signal handlers when
      entering a dialog.

[v2 6/7] xfsdump: convert to the POSIX signal API
    - Was patch #4 from v1 series. Essentially the same,
      but changes in dlog.c are simpler since the signal
      handler doesn't get swapped.

[v2 7/7] xfsdump: refactor inventory session creation
    - New patch to make it obvious that the signal mask
      is properly restored after creating the inventory.


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 v2 1/7] 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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

[PATCH v2 2/7] 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       |   49 ++++++-------------------------------------------
 common/ring.c       |    1 -
 librmt/rmtcommand.c |    7 +------
 librmt/rmtwrite.c   |    5 -----
 4 files changed, 7 insertions(+), 55 deletions(-)

diff --git a/common/main.c b/common/main.c
index c4d6878..d21b559 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;
@@ -547,13 +546,18 @@ main( int argc, char *argv[] )
 	 * be released at pre-emption points and upon pausing in the main
 	 * loop.
 	 */
+
+	/* always ignore SIGPIPE, instead handle EPIPE as part
+	 * of normal sys call error handling
+	 */
+	sigset( SIGPIPE, SIG_IGN );
+
 	if ( ! miniroot && ! pipeline ) {
 		stop_in_progress = BOOL_FALSE;
 		coredump_requested = BOOL_FALSE;
 		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 +567,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 );
@@ -596,7 +598,6 @@ main( int argc, char *argv[] )
 		sigset( SIGINT, sighandler );
 		sigset( SIGHUP, sighandler );
 		sigset( SIGTERM, sighandler );
-		sigset( SIGPIPE, sighandler );
 
 		ok = drive_init2( argc,
 				  argv,
@@ -804,16 +805,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 +880,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 +1119,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 +1157,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 +1582,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 +1610,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 +1642,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

[PATCH v2 3/7] 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 d21b559..b6674f9 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);
 	}
 
@@ -572,8 +571,6 @@ main( int argc, char *argv[] )
 		alarm( 0 );
 		sigset( SIGALRM, sighandler );
 		sighold( SIGALRM );
-		sigset( SIGCLD, sighandler );
-		sighold( SIGCLD );
 	}
 
 	/* do content initialization.
@@ -585,7 +582,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);
 	}
 
@@ -881,9 +877,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 );
@@ -896,10 +890,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 );
@@ -1492,11 +1482,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 ) {
@@ -1516,117 +1501,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
@@ -1643,7 +1545,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

[PATCH v2 4/7] xfsdump: rework dialog timeout and EINTR reliance

[Bill Kendall]

The current dialog code uses alarm(2) to implement the dialog timeout.
This is suboptimal as the main loop already makes use of alarm.

Additionally the existing code relies on its blocking read(2) being
interrupted if a signal is received. This is not guaranteed to work
as a signal may be delivered to a different thread.

Both of these issues can be resolved by using select(2) rather than
a blocking read.

This also changes dlog_desist() to not change 'dlog_ttyfd', as that
could impact a dialog already in progress. Clearing 'dlog_allowed_flag'
is sufficient to disable dialogs.

Signed-off-by: Bill Kendall <wkendall@sgi.com>
---
 common/dlog.c |   57 +++++++++++++++++++++++++++++++++------------------------
 1 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/common/dlog.c b/common/dlog.c
index 6a243ef..cbaa5cb 100644
--- a/common/dlog.c
+++ b/common/dlog.c
@@ -20,6 +20,7 @@
 #include <xfs/jdm.h>
 
 #include <sys/stat.h>
+#include <sys/select.h>
 #include <fcntl.h>
 #include <time.h>
 #include <errno.h>
@@ -131,7 +132,6 @@ void
 dlog_desist( void )
 {
 	dlog_allowed_flag = BOOL_FALSE;
-	dlog_ttyfd = -1;
 }
 
 intgen_t
@@ -373,13 +373,12 @@ 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;
-	intgen_t nread;
+	time32_t now = time( NULL );
+	intgen_t nread = -1;
 	pid_t pid = getpid( );
 
 	/* display the pre-prompt
@@ -399,16 +398,17 @@ promptinput( char *buf,
 #endif /* NOTYET */
 	mlog( MLOG_NORMAL | MLOG_NOLOCK | MLOG_BARE, promptstr );
 
-	/* set up signal handling
+	/* set up timeout
 	 */
-	dlog_signo_received = -1;
 	if ( dlog_timeouts_flag && timeoutix != IXMAX ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sigrelse( SIGALRM );
-		}
-		sigalrm_save = sigset( SIGALRM, sighandler );
-		alarm_save = alarm( ( u_intgen_t )timeout );
+		timeout += now;
+	} else {
+		timeout = TIMEMAX;
 	}
+
+	/* set up signal handling
+	 */
+	dlog_signo_received = -1;
 	if ( sigintix != IXMAX ) {
 		if ( pid == parentpid && ! miniroot ) {
 			( void )sigrelse( SIGINT );
@@ -432,10 +432,27 @@ promptinput( char *buf,
 		sigquit_save = sigset( SIGQUIT, sighandler );
 	}
 
-	/* wait for input, timeout, or interrupt
+	/* wait for input, timeout, or interrupt.
+	 * note we come out of the select() frequently in order to
+	 * check for a signal. the signal may have been handled by the
+	 * the main thread, so we can't rely on the signal waking us
+	 * up from the select().
 	 */
-	ASSERT( dlog_ttyfd >= 0 );
-	nread = read( dlog_ttyfd, buf, bufsz - 1 );
+	while ( now < timeout && dlog_signo_received == -1 && dlog_allowed_flag ) {
+		int rc;
+		fd_set rfds;
+		struct timeval tv = { 0, 100000 }; // 100 ms
+
+		FD_ZERO( &rfds );
+		FD_SET( dlog_ttyfd, &rfds );
+
+		rc = select( dlog_ttyfd + 1, &rfds, NULL, NULL, &tv );
+		if ( rc > 0 && FD_ISSET( dlog_ttyfd, &rfds ) ) {
+			nread = read( dlog_ttyfd, buf, bufsz - 1 );
+			break;
+		}
+		now = time( NULL );
+	}
 
 	/* restore signal handling
 	 */
@@ -461,18 +478,11 @@ promptinput( char *buf,
 			( void )sighold( SIGINT );
 		}
 	}
-	if ( dlog_timeouts_flag && timeoutix != IXMAX ) {
-		( void )alarm( alarm_save );
-		( void )sigset( SIGALRM, sigalrm_save );
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sighold( SIGALRM );
-		}
-	}
 	
 	/* check for timeout or interrupt
 	 */
 	if ( nread < 0 ) {
-		if ( dlog_signo_received == SIGALRM ) {
+		if ( now >= timeout ) {
 			mlog( MLOG_NORMAL | MLOG_NOLOCK | MLOG_BARE,
 			      _("timeout\n") );
 			*exceptionixp = timeoutix;
@@ -496,8 +506,7 @@ promptinput( char *buf,
 			*exceptionixp = sigquitix;
 		} else {
 			mlog( MLOG_NORMAL | MLOG_NOLOCK | MLOG_BARE,
-			      _("unknown signal during dialog: %d\n"),
-			      dlog_signo_received );
+			      _("abnormal dialog termination\n"));
 			*exceptionixp = sigquitix;
 		}
 		return BOOL_FALSE;
-- 
1.7.0.4

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

[PATCH v2 5/7] xfsdump: rework dialog to use main signal handler

[Bill Kendall]

xfsdump currently swaps in a different signal handler for the duration
of a dialog. This patch changes the code to have a global signal
handler which will first give the dialog a chance to process the
signal. If a dialog is not active or if it is not interested in the
signal, the signal will be processed as usual.

There is one side effect to this change. SIGQUIT is now caught in the
miniroot/pipeline case as the handler needs to be setup in case a
dialog needs it. The signal handler will exit in this case just as
if SIGQUIT was not caught, the only real difference being that a
core file will not be generated.

Signed-off-by: Bill Kendall <wkendall@sgi.com>
---
 common/dlog.c |   32 +++++++++++++++++---------------
 common/dlog.h |    3 +++
 common/main.c |    6 ++++++
 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/common/dlog.c b/common/dlog.c
index cbaa5cb..749de1b 100644
--- a/common/dlog.c
+++ b/common/dlog.c
@@ -39,6 +39,8 @@ static bool_t dlog_allowed_flag = BOOL_FALSE;
 static bool_t dlog_timeouts_flag = BOOL_FALSE;
 static char *promptstr = " -> ";
 
+static sigset_t dlog_registered_sigs;
+
 static bool_t promptinput( char *buf,
 			   size_t bufsz,
 			   ix_t *exceptionixp,
@@ -49,7 +51,6 @@ static bool_t promptinput( char *buf,
 			   ix_t sigquitix,
 			   char *fmt, ... );
 static void dlog_string_query_print( void *ctxp, char *fmt, ... );
-static void sighandler( int );
 
 bool_t
 dlog_init( int argc, char *argv[ ] )
@@ -66,6 +67,8 @@ dlog_init( int argc, char *argv[ ] )
 	dlog_allowed_flag = BOOL_TRUE;
 	dlog_timeouts_flag = BOOL_TRUE;
 
+	sigemptyset( &dlog_registered_sigs );
+
 	/* look for command line option claiming the operator knows
 	 * what's up.
 	 */
@@ -341,10 +344,15 @@ dlog_string_ack( char *ackstr[ ], size_t ackcnt )
  */
 static int dlog_signo_received;
 
-static void
-sighandler( int signo )
+bool_t
+dlog_sighandler( int signo )
 {
+	if ( sigismember( &dlog_registered_sigs, signo ) < 1 )
+		return BOOL_FALSE;
+	// only process the first signal
+	sigemptyset( &dlog_registered_sigs );
 	dlog_signo_received = signo;
+	return BOOL_TRUE;
 }
 
 /* ARGSUSED */
@@ -373,10 +381,6 @@ promptinput( char *buf,
 	     ... )
 {
 	va_list args;
-	void (* sigint_save)(int) = NULL;
-	void (* sighup_save)(int) = NULL;
-	void (* sigterm_save)(int) = NULL;
-	void (* sigquit_save)(int) = NULL;
 	time32_t now = time( NULL );
 	intgen_t nread = -1;
 	pid_t pid = getpid( );
@@ -409,27 +413,28 @@ promptinput( char *buf,
 	/* set up signal handling
 	 */
 	dlog_signo_received = -1;
+	sigemptyset( &dlog_registered_sigs );
 	if ( sigintix != IXMAX ) {
 		if ( pid == parentpid && ! miniroot ) {
 			( void )sigrelse( SIGINT );
 		}
-		sigint_save = sigset( SIGINT, sighandler );
+		sigaddset( &dlog_registered_sigs, SIGINT );
 	}
 	if ( sighupix != IXMAX ) {
 		if ( pid == parentpid && ! miniroot ) {
 			( void )sigrelse( SIGHUP );
 		}
-		sighup_save = sigset( SIGHUP, sighandler );
+		sigaddset( &dlog_registered_sigs, SIGHUP );
 		if ( pid == parentpid && ! miniroot ) {
 			( void )sigrelse( SIGTERM );
 		}
-		sigterm_save = sigset( SIGTERM, sighandler );
+		sigaddset( &dlog_registered_sigs, SIGTERM );
 	}
 	if ( sigquitix != IXMAX ) {
 		if ( pid == parentpid && ! miniroot ) {
 			( void )sigrelse( SIGQUIT );
 		}
-		sigquit_save = sigset( SIGQUIT, sighandler );
+		sigaddset( &dlog_registered_sigs, SIGQUIT );
 	}
 
 	/* wait for input, timeout, or interrupt.
@@ -456,24 +461,21 @@ promptinput( char *buf,
 
 	/* restore signal handling
 	 */
+	sigemptyset( &dlog_registered_sigs );
 	if ( sigquitix != IXMAX ) {
-		( void )sigset( SIGQUIT, sigquit_save );
 		if ( pid == parentpid && ! miniroot ) {
 			( void )sighold( SIGQUIT );
 		}
 	}
 	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 );
 		}
 	}
 	if ( sigintix != IXMAX ) {
-		( void )sigset( SIGINT, sigint_save );
 		if ( pid == parentpid && ! miniroot ) {
 			( void )sighold( SIGINT );
 		}
diff --git a/common/dlog.h b/common/dlog.h
index 0ee6331..bc17c41 100644
--- a/common/dlog.h
+++ b/common/dlog.h
@@ -41,6 +41,9 @@ extern void dlog_desist( void );
  */
 extern intgen_t dlog_fd( void );
 
+/* returns BOOL_TRUE if a dialog consumed the given signal
+ */
+extern bool_t dlog_sighandler( int signo );
 
 /* bracket a dialog session
  */
diff --git a/common/main.c b/common/main.c
index b6674f9..825c894 100644
--- a/common/main.c
+++ b/common/main.c
@@ -594,6 +594,7 @@ main( int argc, char *argv[] )
 		sigset( SIGINT, sighandler );
 		sigset( SIGHUP, sighandler );
 		sigset( SIGTERM, sighandler );
+		sigset( SIGQUIT, sighandler );
 
 		ok = drive_init2( argc,
 				  argv,
@@ -1482,6 +1483,11 @@ mrh_sighandler( int signo )
 static void
 sighandler( int signo )
 {
+	/* dialog gets first crack at the signal
+	 */
+	if ( dlog_sighandler( signo ) )
+		return;
+
 	/* if in miniroot, don't do anything risky. just quit.
 	 */
 	if ( miniroot || pipeline ) {
-- 
1.7.0.4

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

[PATCH v2 6/7] 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.

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.

Signed-off-by: Bill Kendall <wkendall@sgi.com>
---
 common/cldmgr.c |    5 ---
 common/dlog.c   |   44 ++++++-------------------
 common/main.c   |   95 ++++++++++++++++++++++++++----------------------------
 common/ring.c   |   14 +++++---
 dump/content.c  |   21 +++++-------
 5 files changed, 74 insertions(+), 105 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 749de1b..59a56bd 100644
--- a/common/dlog.c
+++ b/common/dlog.c
@@ -31,9 +31,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;
@@ -383,7 +380,7 @@ promptinput( char *buf,
 	va_list args;
 	time32_t now = time( NULL );
 	intgen_t nread = -1;
-	pid_t pid = getpid( );
+	sigset_t orig_set;
 
 	/* display the pre-prompt
 	 */
@@ -411,32 +408,28 @@ promptinput( char *buf,
 	}
 
 	/* set up signal handling
+	 * the mlog lock is held for the life of the dialog (see dlog_begin)
+	 * 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 unblock the relevant signals for this thread. note this means
+	 * the current thread or the main thread might handle one of these
+	 * signals.
 	 */
 	dlog_signo_received = -1;
 	sigemptyset( &dlog_registered_sigs );
 	if ( sigintix != IXMAX ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sigrelse( SIGINT );
-		}
 		sigaddset( &dlog_registered_sigs, SIGINT );
 	}
 	if ( sighupix != IXMAX ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sigrelse( SIGHUP );
-		}
 		sigaddset( &dlog_registered_sigs, SIGHUP );
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sigrelse( SIGTERM );
-		}
 		sigaddset( &dlog_registered_sigs, SIGTERM );
 	}
 	if ( sigquitix != IXMAX ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sigrelse( SIGQUIT );
-		}
 		sigaddset( &dlog_registered_sigs, SIGQUIT );
 	}
 
+	sigprocmask( SIG_UNBLOCK, &dlog_registered_sigs, &orig_set );
+
 	/* wait for input, timeout, or interrupt.
 	 * note we come out of the select() frequently in order to
 	 * check for a signal. the signal may have been handled by the
@@ -461,25 +454,8 @@ promptinput( char *buf,
 
 	/* restore signal handling
 	 */
+	sigprocmask( SIG_SETMASK, &orig_set, NULL );
 	sigemptyset( &dlog_registered_sigs );
-	if ( sigquitix != IXMAX ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sighold( SIGQUIT );
-		}
-	}
-	if ( sighupix != IXMAX ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sighold( SIGHUP );
-		}
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sighold( SIGTERM );
-		}
-	}
-	if ( sigintix != IXMAX ) {
-		if ( pid == parentpid && ! miniroot ) {
-			( void )sighold( SIGINT );
-		}
-	}
 	
 	/* check for timeout or interrupt
 	 */
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;
+	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 );
 
 	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.
@@ -591,10 +604,11 @@ main( int argc, char *argv[] )
 	if ( miniroot || pipeline ) {
 		intgen_t exitcode;
 
-		sigset( SIGINT, sighandler );
-		sigset( SIGHUP, sighandler );
-		sigset( SIGTERM, sighandler );
-		sigset( SIGQUIT, sighandler );
+		sa.sa_handler = sighandler;
+		sigaction( SIGINT, &sa, NULL );
+		sigaction( SIGHUP, &sa, NULL );
+		sigaction( SIGTERM, &sa, NULL );
+		sigaction( SIGQUIT, &sa, NULL );
 
 		ok = drive_init2( argc,
 				  argv,
@@ -686,6 +700,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.
@@ -874,16 +889,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 );
 	}
 
@@ -891,14 +898,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
@@ -1071,6 +1073,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
 	 */
@@ -1107,15 +1113,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;
 
@@ -1544,14 +1549,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

[PATCH v2 7/7] xfsdump: refactor inventory session creation

[Bill Kendall]

Move the inventory session creation out of content_init and into a
new function, create_inv_session, in order to simplify restoration
of the signal mask. TTY-related signals are blocked prior to calling
create_inv_session and restored afterwards.

Signed-off-by: Bill Kendall <wkendall@sgi.com>
---
 dump/content.c |  175 +++++++++++++++++++++++++++++--------------------------
 1 files changed, 92 insertions(+), 83 deletions(-)

diff --git a/dump/content.c b/dump/content.c
index fcc952e..9905c88 100644
--- a/dump/content.c
+++ b/dump/content.c
@@ -507,7 +507,13 @@ static quota_info_t quotas[] = {
 
 
 /* definition of locally defined static functions ****************************/
-
+static bool_t create_inv_session(
+		global_hdr_t *gwhdrtemplatep,
+		uuid_t *fsidp,
+		const char *mntpnt,
+		const char *fsdevice,
+		ix_t subtreecnt,
+		size_t strmix);
 
 bool_t
 content_init( intgen_t argc,
@@ -1695,100 +1701,27 @@ baseuuidbypass:
 	}
 
 	/* open the dump inventory and a dump inventory write session
-	 * if an inventory update is to be done. first create a cleanup
-	 * handler, to close the inventory on exit.
+	 * if an inventory update is to be done.
 	 */
 	if ( sc_inv_updatepr ) {
-		char *qmntpnt;
-		char *qfsdevice;
+		bool_t result;
 		sigset_t tty_set, orig_set;
 
-		rval = atexit( inv_cleanup );
-		ASSERT( ! rval );
-		sc_inv_idbtoken = inv_open( ( inv_predicate_t )INV_BY_UUID,
-					    INV_SEARCH_N_MOD,
-					    ( void * )&fsid );
-		if ( sc_inv_idbtoken == INV_TOKEN_NULL ) {
-			return BOOL_FALSE;
-		}
-		qmntpnt = ( char * )calloc( 1, strlen( gwhdrtemplatep->gh_hostname )
-					       +
-					       1
-					       +
-					       strlen( mntpnt )
-					       +
-					       1 );
-		ASSERT( qmntpnt );
-		ASSERT( strlen( gwhdrtemplatep->gh_hostname ));
-		( void )sprintf( qmntpnt,
-				 "%s:%s",
-				 gwhdrtemplatep->gh_hostname,
-				 mntpnt );
-		qfsdevice = ( char * )calloc( 1, strlen( gwhdrtemplatep->gh_hostname )
-					       +
-					       1
-					       +
-					       strlen( fsdevice )
-					       +
-					       1 );
-		ASSERT( qfsdevice );
-		( void )sprintf( qfsdevice,
-				 "%s:%s",
-				 gwhdrtemplatep->gh_hostname,
-				 fsdevice );
-
-		/* hold tty signals while creating a new inventory session
-		 */
+		/* hold tty signals while creating a new inventory session */
 		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,
-						&gwhdrtemplatep->gh_dumpid,
-						gwhdrtemplatep->gh_dumplabel,
-						subtreecnt ? BOOL_TRUE
-							   : BOOL_FALSE,
-						sc_resumepr,
-						( u_char_t )sc_level,
-						drivecnt,
-						gwhdrtemplatep->gh_timestamp,
-						qmntpnt,
-						qfsdevice );
-		if( sc_inv_sestoken == INV_TOKEN_NULL ) {
-			sigprocmask( SIG_SETMASK, &orig_set, NULL );
-			return BOOL_FALSE;
-		}
-
-		/* open an inventory stream for each stream
-		 */
-		sc_inv_stmtokenp = ( inv_stmtoken_t * )calloc( drivecnt,
-							       sizeof( inv_stmtoken_t ));
-		ASSERT( sc_inv_stmtokenp );
-		for ( strmix = 0 ; strmix < drivecnt ; strmix++ ) {
-			drive_t *drivep = drivepp[ strmix ];
-			char *drvpath;
-
-			if ( strcmp( drivep->d_pathname, "stdio" )) {
-				drvpath = path_reltoabs( drivep->d_pathname,
-							 homedir );
-			} else {
-				drvpath = drivep->d_pathname;
-			}
-			sc_inv_stmtokenp[ strmix ] = inv_stream_open( sc_inv_sestoken,
-								      drvpath );
-			if ( strcmp( drivep->d_pathname, "stdio" )) {
-				free( ( void * )drvpath );
-			}
-			if ( sc_inv_stmtokenp[ strmix ] == INV_TOKEN_NULL ) {
-				sigprocmask( SIG_SETMASK, &orig_set, NULL );
-				return BOOL_FALSE;
-			}
-		}
+		result = create_inv_session( gwhdrtemplatep, &fsid, mntpnt,
+					     fsdevice, subtreecnt, strmix );
 
 		sigprocmask( SIG_SETMASK, &orig_set, NULL );
+
+		if ( !result ) {
+			return BOOL_FALSE;
+		}
 	}
 
 	/* set media change flags to FALSE;
@@ -2012,6 +1945,82 @@ content_statline( char **linespp[ ] )
 	return statlinecnt;
 }
 
+static bool_t
+create_inv_session(
+		global_hdr_t *gwhdrtemplatep,
+		uuid_t *fsidp,
+		const char *mntpnt,
+		const char *fsdevice,
+		ix_t subtreecnt,
+		size_t strmix)
+{
+	intgen_t rval;
+	char *qmntpnt;
+	char *qfsdevice;
+
+	/* create a cleanup handler to close the inventory on exit. */
+	rval = atexit( inv_cleanup );
+	ASSERT( ! rval );
+
+	sc_inv_idbtoken = inv_open( ( inv_predicate_t )INV_BY_UUID,
+					INV_SEARCH_N_MOD,
+					( void * )fsidp );
+	if ( sc_inv_idbtoken == INV_TOKEN_NULL ) {
+		return BOOL_FALSE;
+	}
+	qmntpnt = ( char * )calloc( 1, strlen( gwhdrtemplatep->gh_hostname )
+					+ 1 + strlen( mntpnt ) + 1 );
+	ASSERT( qmntpnt );
+	ASSERT( strlen( gwhdrtemplatep->gh_hostname ));
+	sprintf( qmntpnt, "%s:%s", gwhdrtemplatep->gh_hostname, mntpnt );
+	qfsdevice = ( char * )calloc( 1, strlen( gwhdrtemplatep->gh_hostname )
+					 + 1 + strlen( fsdevice ) + 1 );
+	ASSERT( qfsdevice );
+	sprintf( qfsdevice, "%s:%s", gwhdrtemplatep->gh_hostname, fsdevice );
+
+	sc_inv_sestoken = inv_writesession_open( sc_inv_idbtoken,
+						fsidp,
+						&gwhdrtemplatep->gh_dumpid,
+						gwhdrtemplatep->gh_dumplabel,
+						subtreecnt ? BOOL_TRUE
+							   : BOOL_FALSE,
+						sc_resumepr,
+						( u_char_t )sc_level,
+						drivecnt,
+						gwhdrtemplatep->gh_timestamp,
+						qmntpnt,
+						qfsdevice );
+	if ( sc_inv_sestoken == INV_TOKEN_NULL ) {
+		return BOOL_FALSE;
+	}
+
+	/* open an inventory stream for each stream
+	*/
+	sc_inv_stmtokenp = ( inv_stmtoken_t * )
+				calloc( drivecnt, sizeof( inv_stmtoken_t ));
+	ASSERT( sc_inv_stmtokenp );
+	for ( strmix = 0 ; strmix < drivecnt ; strmix++ ) {
+		drive_t *drivep = drivepp[ strmix ];
+		char *drvpath;
+
+		if ( strcmp( drivep->d_pathname, "stdio" )) {
+			drvpath = path_reltoabs( drivep->d_pathname, homedir );
+		} else {
+			drvpath = drivep->d_pathname;
+		}
+		sc_inv_stmtokenp[ strmix ] = inv_stream_open( sc_inv_sestoken,
+								drvpath );
+		if ( strcmp( drivep->d_pathname, "stdio" )) {
+			free( ( void * )drvpath );
+		}
+		if ( sc_inv_stmtokenp[ strmix ] == INV_TOKEN_NULL ) {
+			return BOOL_FALSE;
+		}
+	}
+
+	return BOOL_TRUE;
+}
+
 static void
 mark_set( drive_t *drivep, xfs_ino_t ino, off64_t offset, int32_t flags )
 {
-- 
1.7.0.4

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

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

[Alex Elder]

On Thu, 2011-08-04 at 17:30 -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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Looks good.

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


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

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

[Alex Elder]

On Thu, 2011-08-04 at 17:30 -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.
> 
> Signed-off-by: Bill Kendall <wkendall@sgi.com>

I don't know the real structure of the code well enough
to verify your statement that it "handles EPIPE just fine."

I see that only _rmt_open() calls pipe(2), setting up a
pipeline between an rsh (child process) and the the code
in _rmt_command().  Thereafter, only _rmt_command() and
_rmt_write() write to the write side of that pipe, and
both of those abort the dump if a write doesn't result
in the expected number of bytes being written.

I see only restore_spec() opens a socket, but it closes
it again.  So I guess I don't see any place that I
expect would produce a EPIPE that doesn't handle it
appropriately.

In any case, I assume your statement is right, and
with that in mind I think the change looks good.

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

PS  I'm in the midst of reviewing patch 3 but I'm out
    of time and will have to pick it up again later.


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

Re: [PATCH v2 3/7] xfsdump: remove SIGCHLD handling

[Alex Elder]

On Thu, 2011-08-04 at 17:30 -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.

OK, it took a bit to verify this, but it looks to
me like most of the child thread stuff is inoperative,
because the "is miniroot" flag gets unconditionally
set, and with that set everything is single-threaded.

By not registering a handler for SIGCHLD you are
assuming the default, which is to ignore it.  It
might be better to explicitly set it to SIG_IGN.

Other than that, this looks good.

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

> Signed-off-by: Bill Kendall <wkendall@sgi.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

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

Re: [PATCH v2 4/7] xfsdump: rework dialog timeout and EINTR reliance

[Alex Elder]

On Thu, 2011-08-04 at 17:30 -0500, Bill Kendall wrote:
> The current dialog code uses alarm(2) to implement the dialog timeout.
> This is suboptimal as the main loop already makes use of alarm.
> 
> Additionally the existing code relies on its blocking read(2) being
> interrupted if a signal is received. This is not guaranteed to work
> as a signal may be delivered to a different thread.
> 
> Both of these issues can be resolved by using select(2) rather than
> a blocking read.
> 
> This also changes dlog_desist() to not change 'dlog_ttyfd', as that
> could impact a dialog already in progress. Clearing 'dlog_allowed_flag'
> is sufficient to disable dialogs.

I can tell by inspection that changes to dlog_ttyfd
go hand-in-hand with changes to dlog_allowed_flag.

The value of dlog_ttyfd is only really used by
promptinput(), which is in turn only called by
dlog_string_query() and dlog_multi_query().  The
latter contains this:
        ASSERT( dlog_allowed_flag );
It would be reassuring to have the same assertion
in dlog_string_query().

(I've given up tracing back much further than
that to ensure your statement that "Clearing
'dlog_allowed_flag' is sufficient to disable
dialogs" is correct.)

A few more things below.  I think the "volatile"
thing ought to be changed, but otherwise:

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

> Signed-off-by: Bill Kendall <wkendall@sgi.com>
> ---
>  common/dlog.c |   57 +++++++++++++++++++++++++++++++++------------------------
>  1 files changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/common/dlog.c b/common/dlog.c
> index 6a243ef..cbaa5cb 100644
> --- a/common/dlog.c
> +++ b/common/dlog.c

. . .

> @@ -399,16 +398,17 @@ promptinput( char *buf,
>  #endif /* NOTYET */
>  	mlog( MLOG_NORMAL | MLOG_NOLOCK | MLOG_BARE, promptstr );
>  
> -	/* set up signal handling
> +	/* set up timeout
>  	 */
> -	dlog_signo_received = -1;
>  	if ( dlog_timeouts_flag && timeoutix != IXMAX ) {
> -		if ( pid == parentpid && ! miniroot ) {
> -			( void )sigrelse( SIGALRM );
> -		}
> -		sigalrm_save = sigset( SIGALRM, sighandler );
> -		alarm_save = alarm( ( u_intgen_t )timeout );
> +		timeout += now;
> +	} else {
> +		timeout = TIMEMAX;
>  	}
> +
> +	/* set up signal handling
> +	 */
> +	dlog_signo_received = -1;

You should make dlog_signo_received have a
"volatile" qualifier to ensure the compiler
doesn't optimize out checking whether it
changed while inside your loop.  Same thing
with dlog_allowed_flag I guess, although
honestly I get confused figuring out which
signal hander is in effect...

>  	if ( sigintix != IXMAX ) {
>  		if ( pid == parentpid && ! miniroot ) {
>  			( void )sigrelse( SIGINT );
> @@ -432,10 +432,27 @@ promptinput( char *buf,
>  		sigquit_save = sigset( SIGQUIT, sighandler );
>  	}
>  
> -	/* wait for input, timeout, or interrupt
> +	/* wait for input, timeout, or interrupt.
> +	 * note we come out of the select() frequently in order to
> +	 * check for a signal. the signal may have been handled by the
> +	 * the main thread, so we can't rely on the signal waking us
> +	 * up from the select().
>  	 */
> -	ASSERT( dlog_ttyfd >= 0 );
> -	nread = read( dlog_ttyfd, buf, bufsz - 1 );
> +	while ( now < timeout && dlog_signo_received == -1 && dlog_allowed_flag ) {
> +		int rc;
> +		fd_set rfds;
> +		struct timeval tv = { 0, 100000 }; // 100 ms
> +
> +		FD_ZERO( &rfds );
> +		FD_SET( dlog_ttyfd, &rfds );
> +
> +		rc = select( dlog_ttyfd + 1, &rfds, NULL, NULL, &tv );

You could pass a null pointer as the timeout to select()
if dlog_timeouts is not set, rather than using TIMEMAX,
in order to truly not time out.  (You would otherwise
know you timed out if select() returned 0.)

(This and the next suggestion are just ideas to
consider--I don't care if you do them or not.)

> +		if ( rc > 0 && FD_ISSET( dlog_ttyfd, &rfds ) ) {
> +			nread = read( dlog_ttyfd, buf, bufsz - 1 );
> +			break;
> +		}
> +		now = time( NULL );

You could exploit the non-portable Linux way of updating
timeout with the time left and avoid having to get the
time 10 times per second.

> +	}
>  
>  	/* restore signal handling
>  	 */

. . .

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

Re: [PATCH v2 5/7] xfsdump: rework dialog to use main signal handler

[Alex Elder]

On Thu, 2011-08-04 at 17:30 -0500, Bill Kendall wrote:
> xfsdump currently swaps in a different signal handler for the duration
> of a dialog. This patch changes the code to have a global signal
> handler which will first give the dialog a chance to process the
> signal. If a dialog is not active or if it is not interested in the
> signal, the signal will be processed as usual.

Yay!  I guess I wasn't alone in being confused by
the signal handlers...

> There is one side effect to this change. SIGQUIT is now caught in the
> miniroot/pipeline case as the handler needs to be setup in case a
> dialog needs it. The signal handler will exit in this case just as
> if SIGQUIT was not caught, the only real difference being that a
> core file will not be generated.

Sounds OK to me.

Your dialog signal handler is clever, and I like it.
Again you should make dlog_registered_sigs have a
"volatile" qualifier.

Otherwise this looks good.

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

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


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

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

[Alex Elder]

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

Re: [PATCH v2 7/7] xfsdump: refactor inventory session creation

[Alex Elder]

On Thu, 2011-08-04 at 17:30 -0500, Bill Kendall wrote:
> Move the inventory session creation out of content_init and into a
> new function, create_inv_session, in order to simplify restoration
> of the signal mask. TTY-related signals are blocked prior to calling
> create_inv_session and restored afterwards.
> 
> Signed-off-by: Bill Kendall <wkendall@sgi.com>

Looks good.

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


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

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

[Bill Kendall]

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

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

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

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

[Christoph Hellwig]

On Fri, Aug 12, 2011 at 02:15:58PM -0500, Bill Kendall wrote:
> >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.

sigaction actuall is a rare case where this is harmful.  To quote
the manpage:

	On some architectures a union is involved:  do not assign to both
	sa_handler and sa_sigaction.

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

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

[Bill Kendall]

On 08/12/2011 03:45 PM, Christoph Hellwig wrote:
> On Fri, Aug 12, 2011 at 02:15:58PM -0500, Bill Kendall wrote:
>>> 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.
>
> sigaction actuall is a rare case where this is harmful.  To quote
> the manpage:
>
> 	On some architectures a union is involved:  do not assign to both
> 	sa_handler and sa_sigaction.

Thanks, I do remember reading this now. I'll leave the code as is,
which sets sa_handler instead of sa_sigaction, and does not touch
the obsolete (possibly undefined) sa_restorer. The rest of the
fields are initialized.

Bill

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