[Findutils-patches] [PATCH 3/3] POSIX compliance fixes for SIGUSR1/SIGUSR2 handling.

[James Youngman]

* doc/find.texi (Controlling Parallelism): SIGUSR1/2 only work if
--max-procs was specified.  Indicate that this is a POSIX requirement.
* xargs/xargs.1 (ASYNCHRONOUS EVENTS): New section; describe handling
of SIGUSR1 and SIGUSR2.  Indicate that the handling of signals in child
processes is not affected.
* xargs/xargs.c (main): Don't set signal handlers for SIGUSR1 and SIGUSR2
unless the --max-procs option was given. Save the initial disposition of
SIGUSR1, SIGUSR1 and the  signal mask we inherited from the parent in
sigact_usr1, sigact_usr2 and parent_signal_mask.
(xargs_do_exec): continue to wait for children to exit as long as there
are too many running (this is a bugfix which eliminates an assertion
failure).
(wait_for_proc): We still need to continue waiting for more children to
exit if too many are still running, even if we received SIGUSR1.

Signed-off-by: James Youngman <address@hidden>
---
 doc/find.texi |   12 ++++-
 xargs/xargs.1 |   73 +++++++++++++++++++++++++++++-----
 xargs/xargs.c |  122 +++++++++++++++++++++++++++++++++++++++++++++------------
 3 files changed, 168 insertions(+), 39 deletions(-)

diff --git a/doc/find.texi b/doc/find.texi
index e6c93ad..b9f33b3 100644
--- a/doc/find.texi
+++ b/doc/find.texi
@@ -2440,6 +2440,7 @@ the argument length limit for exec, less the size of your 
environment,
 less 2048 bytes of headroom.  If this value is more than 128KiB,
 128Kib is used as the default value; otherwise, the default value is
 the maximum.
address@hidden table
 
 @node Controlling Parallelism
 @subsubsection Controlling Parallelism
@@ -2447,7 +2448,7 @@ the maximum.
 Normally, @code{xargs} runs one command at a time.  This is called
 "serial" execution; the commands happen in a series, one after another.
 If you'd like @code{xargs} to do things in "parallel", you can ask it
-to do so, either when you invoke it, or later while it is running.
+to do so by using the @samp{--max-procs} option.
 Running several commands at one time can make the entire operation
 go more quickly, if the commands are independent, and if your system
 has enough resources to handle the load.  When parallelism works in
@@ -2524,9 +2525,9 @@ the parallelism from 4 to 3).  The second @code{kill} 
will reduce it from
 3 to 2.  (@code{%4} works in some shells as a shorthand for the process
 ID of the background job labeled @code{[4]}.)
 
-Similarly, if you started a long xargs job without parallelism, you
+Similarly, if you started a long xargs job with @samp{--max-procs=1}, you
 can easily switch it to start running two commands in parallel by sending
-it a @code{SIGUSR1}.
+it @code{SIGUSR1}.
 
 @code{xargs} will never terminate any existing commands when you ask it
 to run fewer processes.  It merely waits for the excess commands to
@@ -2541,6 +2542,11 @@ a signal, observing the result, then sending the next 
one; or merely by
 delaying for a few seconds between signals (unless your system is very
 heavily loaded).
 
+The POSIX standard requires that @code{xargs} should terminate when
+SIGUSR1 or SIGUSR2 is received; if you don't use the
address@hidden option at all, this is what will happen if you send
+either of these signals.
+
 Whether or not parallel execution will work well for you depends on
 the nature of the commmand you are running in parallel, on the
 configuration of the system on which you are running the command, and
diff --git a/xargs/xargs.1 b/xargs/xargs.1
index 01e1635..fec624c 100644
--- a/xargs/xargs.1
+++ b/xargs/xargs.1
@@ -298,18 +298,55 @@ will run as many processes as
 possible at a time.  Use the 
 .B \-n
 option with 
-.BR \-P ;
-otherwise chances are that only one exec will be done.
-While
+.BR \-\-max-procs ;
+otherwise chances are that only one exec will be done.  See also
+the ASYNCHRONOUS EVENTS section for information on real-time control
+of the degree of parallelism.
+
+.SH "ASYNCHRONOUS EVENTS"
+This section describes how 
+.B xargs 
+handles the receipt of signals. 
+.TP 
+\fBSIGUSR1, SIGUSR2\fR
+If the 
+.B \-\-max-procs 
+option (or equivalently the 
+.B \-P
+option) was not specified, then 
 .B xargs
-is running, you can
-send its process
-a SIGUSR1 signal to increase the number of commands to run simultaneously,
-or a SIGUSR2 to decrease the number.  You cannot decrease it below 1.
+will terminate abnormally, as required by POSIX.  If the
+(non-POSIX-specified) 
+.B \-\-max-procs
+option was specified, SIGUSR1 and SIGUSR2 can be used to control the
+degree of parallelism used by 
+.BR xargs .
+
+Receipt of SIGUSR1 causes 
 .B xargs
-never terminates its commands; when asked to decrease, it merely
+to increase the number of commands to run simultaneously,
+and receipt of SIGUSR2 decreases the number.  You cannot decrease it below 1.
+.B xargs
+never forcibly terminates its commands; when asked to decrease the
+degree of parallelism, it merely
 waits for more than one existing command to terminate before starting
-another.
+another.  If 
+.B xargs
+is started with 
+.B \-\-max-procs=0
+and then SIGUSR1 is received, 
+.B xargs
+will begin to behave as if it had been invoked with 
+.BR \-\-max-procs=1 ,
+that is, commands will be executed serially instead of in parallel.
+
+
+Child processes will be executed with SIGUSR1 and SIGUSR1 handling as
+inherited from 
+.BR xargs 's 
+own parent process.
+.P
+The standard action is taken for all other signals.  
 .SH "EXAMPLES"
 .nf
 .B find /tmp \-name core \-type f \-print | xargs /bin/rm \-f
@@ -388,7 +425,9 @@ exits with the following status:
 Exit codes greater than 128 are used by the shell to indicate that 
 a program died due to a fatal signal.
 .SH "STANDARDS CONFORMANCE"
-As of GNU xargs version 4.2.9, the default behaviour of
+As of GNU 
+.B xargs 
+version 4.2.9, the default behaviour of
 .B xargs
 is not to have a logical end-of-file marker.  POSIX (IEEE Std 1003.1,
 2004 Edition) allows this.
@@ -407,7 +446,13 @@ is that small.  The
 .B \-\-show\-limits 
 option can be used to discover the actual limits in force on the
 current system.
-
+.P
+As of GNU 
+.B xargs
+version 4.5.3, use of the 
+.B \-\-max-procs
+option causes non-POSIX-compliant handling of the SIGUSR1 and SIGUSR2
+signals; see the ASYNCHRONOUS EVENTS section.
 
 .SH "SEE ALSO"
 \fBfind\fP(1), \fBlocate\fP(1), \fBlocatedb\fP(5), \fBupdatedb\fP(1),
@@ -478,6 +523,12 @@ The problem doesn't occur with the output of
 .BR find (1) 
 because it emits just one filename per line.
 .P
+Some algorithms used by 
+.B xargs
+are linear in the number of processes specified in the 
+.B \-\-max-procs 
+option.
+.P
 The best way to report a bug is to use the form at
 http://savannah.gnu.org/bugs/?group=findutils.  
 The reason for this is that you will then be able to track progress in
diff --git a/xargs/xargs.c b/xargs/xargs.c
index 1791a6a..6badde3 100644
--- a/xargs/xargs.c
+++ b/xargs/xargs.c
@@ -208,6 +208,10 @@ static boolean initial_args = true;
    at once.  */
 /* TODO: check conversion safety (i.e. range) for -P option. */
 static volatile sig_atomic_t proc_max = 1;
+static boolean install_user_signal_handlers = false;
+static struct sigaction sigact_usr1;
+static struct sigaction sigact_usr2;
+static sigset_t parent_signal_mask;
 
 /* Did we fork a child yet? */
 static boolean procs_executed = false;
@@ -419,6 +423,38 @@ main (int argc, char **argv)
   enum { XARGS_POSIX_HEADROOM = 2048u };
   
   struct sigaction sigact;
+  sigset_t signal_mask;
+
+  /* Save the signal mask we inherited from our parent so that 
+   * our children can inherit it too. 
+   */
+  if (sigprocmask (SIG_UNBLOCK, NULL, &parent_signal_mask))
+    error(1, errno, _("Failed to determine signal mask"));
+
+#ifdef SIGUSR1
+#ifdef SIGUSR2
+  /* Find out whether SIGUSR1/SIGUSR2 is ignored so that our child 
+   * processes can inherit the same setting. 
+   */
+  if (sigaction(SIGUSR1, NULL, &sigact_usr1))
+    error(1, errno, _("Failed to determine signal status for SIGUSR1"));
+  if (sigaction(SIGUSR2, NULL, &sigact_usr2))
+    error(1, errno, _("Failed to determine signal status for SIGUSR2"));
+  
+  /* Block SIGUSR1 and SIGUSR2 for now, because if the --max-procs
+   * option was specified, we may be sent those signals.  Until we 
+   * have completed option processing, we don't know whether we want 
+   * to catch those signals, and if --max-procs was set, we don't 
+   * want to terminate the program on SIGUSR[12].  This mitigates 
+   * a race condition but does not eliminate it; however, the race 
+   * condition is not a POSIX compliance issue.
+   */
+  sigemptyset (&signal_mask);
+  sigaddset (&signal_mask, SIGUSR1);
+  sigaddset (&signal_mask, SIGUSR2);
+  sigprocmask (SIG_BLOCK, &signal_mask, NULL);
+#endif
+#endif
 
   program_name = argv[0];
   original_exit_value = 0;
@@ -621,6 +657,7 @@ main (int argc, char **argv)
        case 'P':
          /* Allow only up to LONG_MAX child processes. */
          proc_max = parse_num (optarg, 'P', 0L, LONG_MAX, 1);
+         install_user_signal_handlers = true;
          break;
 
         case 'a':
@@ -649,20 +686,34 @@ main (int argc, char **argv)
 
 #ifdef SIGUSR1
 #ifdef SIGUSR2
-  /* Accept signals to increase or decrease the number of running
-     child processes.  Do this as early as possible after setting
-     proc_max.  */
-  sigact.sa_handler = increment_proc_max;
-  sigemptyset(&sigact.sa_mask);
-  sigact.sa_flags = 0;
-  if (0 != sigaction (SIGUSR1, &sigact, (struct sigaction *)NULL))
-         error (0, errno, _("Cannot set SIGUSR1 signal handler"));
-
-  sigact.sa_handler = decrement_proc_max;
-  sigemptyset(&sigact.sa_mask);
-  sigact.sa_flags = 0;
-  if (0 != sigaction (SIGUSR2, &sigact, (struct sigaction *)NULL))
-         error (0, errno, _("Cannot set SIGUSR2 signal handler"));
+  if (install_user_signal_handlers)
+    {
+      /* Accept signals to increase or decrease the number of running
+       * child processes.  Do this as early as possible after setting
+       * proc_max.  This behaviour is not POSIX compliant, since POSIX
+       * requires specific default behaviour of xargs when these
+       * signals are received.  However, since the -max-procs option
+       * is itself non-POSIX, we're not obliged to comply when that
+       * option is in use.
+       */
+      sigact.sa_handler = increment_proc_max;
+      sigemptyset(&sigact.sa_mask);
+      sigact.sa_flags = 0;
+      if (0 != sigaction (SIGUSR1, &sigact, (struct sigaction *)NULL))
+       error (0, errno, _("Cannot set SIGUSR1 signal handler"));
+      
+      sigact.sa_handler = decrement_proc_max;
+      sigemptyset(&sigact.sa_mask);
+      sigact.sa_flags = 0;
+      if (0 != sigaction (SIGUSR2, &sigact, (struct sigaction *)NULL))
+       error (0, errno, _("Cannot set SIGUSR2 signal handler"));
+    }
+
+  /* Unblock SIGUSR1 and SIGUSR2.  If --max-procs was specified, we want 
+   * our signal handlers to run.  If not, we want to reinstate the default 
+   * POSIX behaviour (abnormal termination).
+   */
+  sigprocmask (SIG_UNBLOCK, &signal_mask, NULL);
 #endif /* SIGUSR2 */
 #endif /* SIGUSR1 */
 
@@ -1091,12 +1142,28 @@ print_args (boolean ask)
 }
 
 
-/* Close stdin and attach /dev/null to it.
- * This resolves Savannah bug #3992.
+/* Close stdin and attach /dev/null to it; this resolves Savannah bug #3992.
+ * Restore xargs' parent's SIG_IGN state for SIGUSR[12] and the signal mask.
  */
 static void
 prep_child_for_exec (void)
 {
+#ifdef SIGUSR1
+#ifdef SIGUSR2
+  /* Restore the signal handling (i.e. SIG_IGN, and signal mask)
+   * settings from xargs' own parent.  Don't exit fatally if this
+   * fails because this could be confused (for now) with the utility
+   * we should exec doing the same.
+   */
+  if (sigaction(SIGUSR1, &sigact_usr1, NULL))
+    error(0, errno, _("Failed to restore signal action for SIGUSR1"));
+  if (sigaction(SIGUSR2, &sigact_usr2, NULL))
+    error(0, errno, _("Failed to restore signal action for SIGUSR2"));
+#endif
+#endif
+  if (sigprocmask (SIG_SETMASK, &parent_signal_mask, NULL))
+    error(0, errno, _("Failed to restore signal mask"));
+
   if (!keep_stdin)
     {
       const char inputfile[] = "/dev/null";
@@ -1136,9 +1203,8 @@ xargs_do_exec (const struct buildcmd_control *ctl, struct 
buildcmd_state *state)
     {
       if (proc_max)
        {
-         if (procs_executing >= proc_max)
+         while (procs_executing >= proc_max)
            wait_for_proc (false, proc_max - procs_executing + 1);
-         assert (procs_executing < proc_max);
        }
       if (!query_before_executing && print_command)
        print_args (false);
@@ -1166,6 +1232,9 @@ xargs_do_exec (const struct buildcmd_control *ctl, struct 
buildcmd_state *state)
 
        case 0:         /* Child.  */
          prep_child_for_exec();
+         /* The execvp() syscall will reset signal handlers in the child to 
+          * SIG_DFL, so we don't need to do it.
+          */
          execvp (bc_state.cmd_argv[0], bc_state.cmd_argv);
          error (0, errno, "%s", bc_state.cmd_argv[0]);
          _exit (errno == ENOENT ? 127 : 126);
@@ -1264,13 +1333,16 @@ wait_for_proc (boolean all, unsigned int minreap)
 
              if (stop_waiting && !all)
                {
-                 /* Receipt of SIGUSR1 gave us an extra slot and we
-                  * don't need to wait for all processes to finish.
-                  * We can stop reaping now, but in any case check for 
-                  * further dead children without waiting for another 
-                  * to exit.
-                  */
-                 wflags = WNOHANG;
+                 if (procs_executing < proc_max)
+                   {
+                     /* Receipt of SIGUSR1 gave us an extra slot and
+                      * we don't need to wait for all processes to
+                      * finish.  We can stop reaping now, but in any
+                      * case check for further dead children without
+                      * waiting for another to exit.
+                      */
+                     wflags = WNOHANG;
+                   }
                }
            }
          
-- 
1.5.6.5