From db9d9f76b478d4d2c599dcdf7939814d9ef9c97a Mon Sep 17 00:00:00 2001 From: jay Date: Mon, 7 Jan 2008 01:23:48 +0000 Subject: [PATCH] Reap child processes sooner in order to reduce resource usage --- ChangeLog | 28 +++++++++++ NEWS | 5 ++ xargs/xargs.c | 150 +++++++++++++++++++++++++++++++++++++++++----------------- 3 files changed, 139 insertions(+), 44 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4e73a22..8744400 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,31 @@ +2008-01-07 James Youngman + + * xargs/xargs.c: (main): Standardise on "Warning" instead of + "warning" in messages. + + * xargs/xargs.c: (add_proc): Use x2nrealloc to extend the pids + array, rather than doubling the size of the buffer (since the old + aproach was vulnerable to overflow). + + Reap all available child processes before every fork. This fixes + Savannah bug #21960. + * xargs/xargs.c: (proc_max): since this is a non-negative + quantity, make it unsigned. + (procs_executing): Likewise. + (pids_alloc): Likewise (using size_t). + (procs_executed): In order to prevent possible overflow, make this + a boolean, not a count. We only cared if the previous counter was + zero or not, anwyay. + (add_proc): Set procs_executed to true rather than incrementing it. + (wait_for_proc): When called, always reap all available children. + Add an extra argument which is the minimum number of children we + must reap before returning. + (wait_for_proc_all): Pass the new extra argument. + (xargs_do_exec): Call wait_for_proc() to reap all available + children before forking a new child. Modify other calls to + wait_for_proc to pass the new extra argument. + (NEWS): Mention this change. + 2007-12-20 James Youngman * find/fstype.c, find/ftsfind.c, find/parser.c, find/pred.c, diff --git a/NEWS b/NEWS index 3fa84db..df51eaf 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,11 @@ GNU findutils NEWS - User visible changes. -*- outline -*- (allout) * Major changes in release 4.3.13-CVS +** Bug Fixes +#21960: xargs should collect the exit status of child processes even if +the total count of unreaped children has not yet reached the maximum +allowed. + * Major changes in release 4.3.12, 2007-12-19 ** Bug Fixes diff --git a/xargs/xargs.c b/xargs/xargs.c index 1394fa3..1365a71 100644 --- a/xargs/xargs.c +++ b/xargs/xargs.c @@ -1,6 +1,6 @@ /* xargs -- build and execute command lines from standard input Copyright (C) 1990, 91, 92, 93, 94, 2000, 2003, 2004, 2005, 2006, - 2007 Free Software Foundation, Inc. + 2007, 2008 Free Software Foundation, Inc. This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -206,19 +206,19 @@ static boolean initial_args = true; /* If nonzero, the maximum number of child processes that can be running at once. */ -static int proc_max = 1; +static unsigned long int proc_max = 1uL; -/* Total number of child processes that have been executed. */ -static int procs_executed = 0; +/* Did we fork a child yet? */ +static boolean procs_executed = false; /* The number of elements in `pids'. */ -static int procs_executing = 0; +static unsigned long int procs_executing = 0uL; /* List of child processes currently executing. */ static pid_t *pids = NULL; /* The number of allocated elements in `pids'. */ -static int pids_alloc = 0; +static size_t pids_alloc = 0u; /* Exit status; nonzero if any child process exited with a status of 1-125. */ @@ -267,7 +267,7 @@ static boolean print_args PARAMS ((boolean ask)); static int xargs_do_exec (const struct buildcmd_control *cl, struct buildcmd_state *state); static void exec_if_possible PARAMS ((void)); static void add_proc PARAMS ((pid_t pid)); -static void wait_for_proc PARAMS ((boolean all)); +static void wait_for_proc PARAMS ((boolean all, unsigned int minreap)); static void wait_for_proc_all PARAMS ((void)); static long parse_num PARAMS ((char *str, int option, long min, long max, int fatal)); static void usage PARAMS ((FILE * stream)); @@ -580,7 +580,7 @@ main (int argc, char **argv) if (arg_size > bc_ctl.posix_arg_size_max) { error (0, 0, - _("warning: value %ld for -s option is too large, " + _("Warning: value %ld for -s option is too large, " "using %ld instead"), arg_size, bc_ctl.posix_arg_size_max); arg_size = bc_ctl.posix_arg_size_max; @@ -611,7 +611,8 @@ main (int argc, char **argv) break; case 'P': - proc_max = parse_num (optarg, 'P', 0L, -1L, 1); + /* Allow only up to LONG_MAX child processes. */ + proc_max = parse_num (optarg, 'P', 0L, LONG_MAX, 1); break; case 'a': @@ -747,7 +748,7 @@ main (int argc, char **argv) /* SYSV xargs seems to do at least one exec, even if the input is empty. */ if (bc_state.cmd_argc != bc_ctl.initial_argc - || (always_run_command && procs_executed == 0)) + || (always_run_command && !procs_executed)) xargs_do_exec (&bc_ctl, &bc_state); } @@ -951,7 +952,7 @@ read_line (void) { /* This is just a warning message. We only issue it once. */ error (0, 0, - _("warning: a NUL character occurred in the input. " + _("Warning: a NUL character occurred in the input. " "It cannot be passed through in the argument list. " "Did you mean to use the --null option?")); nullwarning_given = 1; @@ -1105,14 +1106,31 @@ xargs_do_exec (const struct buildcmd_control *ctl, struct buildcmd_state *state) if (!query_before_executing || print_args (true)) { - if (proc_max && procs_executing >= proc_max) - wait_for_proc (false); + if (proc_max) + { + if (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); + + /* Before forking, reap any already-exited child. We do this so + that we don't leave unreaped children around while we build a + new command line. For example this command will spend most + of its time waiting for sufficient arguments to launch + another command line: + + seq 1 1000 | fmt | while read x ; do echo $x; sleep 1 ; done | + ./xargs -P 200 -n 20 sh -c 'echo "$@"; sleep $((1 + $RANDOM % 5))' sleeper + */ + wait_for_proc (false, 0u); + /* If we run out of processes, wait for a child to return and try again. */ while ((child = fork ()) < 0 && errno == EAGAIN && procs_executing) - wait_for_proc (false); + wait_for_proc (false, 1u); + switch (child) { case -1: @@ -1149,60 +1167,107 @@ exec_if_possible (void) static void add_proc (pid_t pid) { - int i; + unsigned int i, j; /* Find an empty slot. */ for (i = 0; i < pids_alloc && pids[i]; i++) ; + + /* Extend the array if we failed. */ if (i == pids_alloc) { - if (pids_alloc == 0) - { - pids_alloc = proc_max ? proc_max : 64; - pids = xmalloc (sizeof (pid_t) * pids_alloc); - } - else - { - pids_alloc *= 2; - pids = xrealloc (pids, - sizeof (pid_t) * pids_alloc); - } - memset (&pids[i], '\0', sizeof (pid_t) * (pids_alloc - i)); + pids = x2nrealloc (pids, &pids_alloc, sizeof *pids); + + /* Zero out the new slots. */ + for (j=i; j= minreap) + { + /* We already reaped enough children. To save system + * resources, reap any dead children anyway, but do not + * wait for any currently executing children to exit. + + */ + wflags = WNOHANG; + } + } + do + { + /* Wait for any child. We used to use wait() here, but it's + * unlikely that that offers any portability advantage over + * wait these days. + */ + while ((pid = waitpid (-1, &status, wflags)) == (pid_t) -1) + { + if (errno != EINTR) + error (1, errno, _("error waiting for child process")); + } + /* Find the entry in `pids' for the child process that exited. */ - for (i = 0; i < pids_alloc && pid != pids[i]; i++) - ; + if (pid) + { + for (i = 0; i < pids_alloc && pid != pids[i]; i++) + ; + } } - while (i == pids_alloc); /* A child died that we didn't start? */ + while (pid && i == pids_alloc); /* A child died that we didn't start? */ + if (!pid) + { + if (!(wflags & WNOHANG)) + { + /* Nothing remained to be reaped. This should not + * happen, because procs_executing should contain the + * number of child processes still executing, so the + * loop should have terminated. + */ + error (0, 0, _("Warning: Lost track of %d child processes"), + procs_executing); + } + else + { + /* Children are (probably) executing but are not ready + * to be reaped at the moment. + */ + } + break; + } + /* Remove the child from the list. */ pids[i] = 0; procs_executing--; + reaped++; if (WEXITSTATUS (status) == 126 || WEXITSTATUS (status) == 127) exit (WEXITSTATUS (status)); /* Can't find or run the command. */ @@ -1214,9 +1279,6 @@ wait_for_proc (boolean all) error (125, 0, _("%s: terminated by signal %d"), bc_state.cmd_argv[0], WTERMSIG (status)); if (WEXITSTATUS (status) != 0) child_error = 123; - - if (!all) - break; } } @@ -1231,7 +1293,7 @@ wait_for_proc_all (void) return; waiting = true; - wait_for_proc (true); + wait_for_proc (true, 0u); waiting = false; if (original_exit_value != child_error) -- 2.11.4.GIT