Change to xargs to avoid orphaning of utility processes on
signal|exit 255 from child
Matthew Story
matthewstory at gmail.com
Mon Mar 12 01:39:11 UTC 2012
On Sat, Mar 10, 2012 at 1:23 PM, Matthew Story <matthewstory at gmail.com>wrote:
> On Fri, Mar 9, 2012 at 11:27 PM, Matthew Story <matthewstory at gmail.com>wrote:
>
>> On Fri, Mar 9, 2012 at 6:13 PM, Matthew Story <matthewstory at gmail.com>wrote:
>>
>>> On Fri, Mar 9, 2012 at 5:54 PM, Jilles Tjoelker <jilles at stack.nl>wrote:
>>>
>>>> On Tue, Feb 21, 2012 at 12:50:19PM -0500, Matthew Story wrote:
>>>> > On Thu, Feb 16, 2012 at 7:09 PM, Matthew Story <
>>>> matthewstory at gmail.com>wrote:
>>>> > > Apologies if this is the wrong list, I would like to submit a patch
>>>> that
>>>> > > changes the behavior of xargs(1) on signal to child utility process
>>>> or
>>>> > > child utility process exiting 255. The patch(es) is|are available
>>>> here:
>>>>
>>>> [...snip]
>>>>
>>>> > This would cause xargs to wait on children if the command line cannot
>>>> be
>>>> > assembled, or utility cannot be invoked, in addition to the 2 cases
>>>> covered
>>>> > by the patch. This should leave termination via signal to the xargs
>>>>
>>> > process itself as the only outstanding case where xargs orphans
>>>> utilities,
>>>> > which is congruent with sh(1) behavior, and still allows for
>>>> signaling all
>>>> > processes via signal to the process group, if you actually desire to
>>>> signal
>>>> > all utility processes, along with xargs itself. Thoughts?
>>>>
>>>> I think that makes sense.
>>>>
>>>> I updated the patch to the recent changes in -current and changed a
>>>> local from short to int:
>>>> [...snip]
>>>
>>> changes look good to me, thanks for cleaning it up for -CURRENT. I
>>> will provide an additional patch to cover waiting for the other 2 cases
>>> (cannot assemble, or cannot invoke) later tonight after some more testing.
>>> [...snip]
>>>
>>
>> I believe I have finished removing the orphan cases for the assembly
>> failure and invocation failure cases, updated patch attached for review,
>> also available here
>>
>>
>> http://axe0.blackskyresearch.net/patches/matt/xargs.no_orphan.full.patch.txt
>>
>
> Updated this patch (also attached), to consolidate all waitchildren, exit
> to use xexit, and tidied up a multiple warning condition on childerr to
> warn only once while waiting to exit.
>
Sorry for the noisy updates, but i made the same branching mistake here as
in my very first patch, let me know if there are any more issues with this
one. (updated the above link, along with attachment).
>
>
>>
>> This patch includes the cleaned-up changes Jilles sent back earlier
>> (thanks again).
>>
>> --
>> regards,
>> matt
>>
>
>
>
> --
> regards,
> matt
>
--
regards,
matt
-------------- next part --------------
Index: xargs.1
===================================================================
--- xargs.1 (revision 232845)
+++ xargs.1 (working copy)
@@ -294,17 +294,17 @@
.Ar utility
reads from the standard input.
.Pp
-The
-.Nm
-utility exits immediately (without processing any further input) if a
-command line cannot be assembled,
+If a command line cannot be assembled, or
+cannot be invoked, or if an invocation of
.Ar utility
-cannot be invoked, an invocation of
-.Ar utility
is terminated by a signal,
or an invocation of
.Ar utility
-exits with a value of 255.
+exits with a value of 255, the
+.Nm
+utility stops processing input and exits after all invocations of
+.Ar utility
+finish processing.
.Sh EXIT STATUS
The
.Nm
Index: xargs.c
===================================================================
--- xargs.c (revision 232845)
+++ xargs.c (working copy)
@@ -70,6 +70,7 @@
static void usage(void);
void strnsubst(char **, const char *, const char *, size_t);
static pid_t xwait(int block, int *status);
+static void xexit(const char *, const int);
static void waitchildren(const char *, int);
static void pids_init(void);
static int pids_empty(void);
@@ -280,10 +281,8 @@
switch (ch = getchar()) {
case EOF:
/* No arguments since last exec. */
- if (p == bbp) {
- waitchildren(*av, 1);
- exit(rval);
- }
+ if (p == bbp)
+ xexit(*av, rval);
goto arg1;
case ' ':
case '\t':
@@ -308,8 +307,10 @@
count++; /* Indicate end-of-line (used by -L) */
/* Quotes do not escape newlines. */
-arg1: if (insingle || indouble)
- errx(1, "unterminated quote");
+arg1: if (insingle || indouble) {
+ warnx("unterminated quote");
+ xexit(*av, 1);
+ }
arg2:
foundeof = *eofstr != '\0' &&
strncmp(argp, eofstr, p - argp) == 0;
@@ -342,8 +343,10 @@
*/
inpline = realloc(inpline, curlen + 2 +
strlen(argp));
- if (inpline == NULL)
- errx(1, "realloc failed");
+ if (inpline == NULL) {
+ warnx("realloc failed");
+ xexit(*av, 1);
+ }
if (curlen == 1)
strcpy(inpline, argp);
else
@@ -360,17 +363,17 @@
*/
if (xp == endxp || p > ebp || ch == EOF ||
(Lflag <= count && xflag) || foundeof) {
- if (xflag && xp != endxp && p > ebp)
- errx(1, "insufficient space for arguments");
+ if (xflag && xp != endxp && p > ebp) {
+ warnx("insufficient space for arguments");
+ xexit(*av, 1);
+ }
if (jfound) {
for (avj = argv; *avj; avj++)
*xp++ = *avj;
}
prerun(argc, av);
- if (ch == EOF || foundeof) {
- waitchildren(*av, 1);
- exit(rval);
- }
+ if (ch == EOF || foundeof)
+ xexit(*av, rval);
p = bbp;
xp = bxp;
count = 0;
@@ -394,8 +397,10 @@
if (zflag)
goto addch;
/* Backslash escapes anything, is escaped by quotes. */
- if (!insingle && !indouble && (ch = getchar()) == EOF)
- errx(1, "backslash at EOF");
+ if (!insingle && !indouble && (ch = getchar()) == EOF) {
+ warnx("backslash at EOF");
+ xexit(*av, 1);
+ }
/* FALLTHROUGH */
default:
addch: if (p < ebp) {
@@ -404,11 +409,15 @@
}
/* If only one argument, not enough buffer space. */
- if (bxp == xp)
- errx(1, "insufficient space for argument");
+ if (bxp == xp) {
+ warnx("insufficient space for argument");
+ xexit(*av, 1);
+ }
/* Didn't hit argument limit, so if xflag object. */
- if (xflag)
- errx(1, "insufficient space for arguments");
+ if (xflag) {
+ warnx("insufficient space for arguments");
+ xexit(*av, 1);
+ }
if (jfound) {
for (avj = argv; *avj; avj++)
@@ -449,16 +458,20 @@
* a NULL at the tail.
*/
tmp = malloc((argc + 1) * sizeof(char**));
- if (tmp == NULL)
- errx(1, "malloc failed");
+ if (tmp == NULL) {
+ warnx("malloc failed");
+ xexit(*argv, 1);
+ }
tmp2 = tmp;
/*
* Save the first argument and iterate over it, we
* cannot do strnsubst() to it.
*/
- if ((*tmp++ = strdup(*avj++)) == NULL)
- errx(1, "strdup failed");
+ if ((*tmp++ = strdup(*avj++)) == NULL) {
+ warnx("strdup failed");
+ xexit(*argv, 1);
+ }
/*
* For each argument to utility, if we have not used up
@@ -475,8 +488,10 @@
if (repls > 0)
repls--;
} else {
- if ((*tmp = strdup(*tmp)) == NULL)
- errx(1, "strdup failed");
+ if ((*tmp = strdup(*tmp)) == NULL) {
+ warnx("strdup failed");
+ xexit(*argv, 1);
+ }
tmp++;
}
}
@@ -547,7 +562,8 @@
childerr = 0;
switch (pid = vfork()) {
case -1:
- err(1, "vfork");
+ warn("vfork");
+ xexit(*argv, 1);
case 0:
if (oflag) {
if ((fd = open(_PATH_TTY, O_RDONLY)) == -1)
@@ -593,26 +609,45 @@
}
static void
+xexit(const char *name, const int exit_code) {
+ waitchildren(name, 1);
+ exit(exit_code);
+}
+
+static void
waitchildren(const char *name, int waitall)
{
pid_t pid;
int status;
+ int cause_exit = 0;
while ((pid = xwait(waitall || pids_full(), &status)) > 0) {
- /* If we couldn't invoke the utility, exit. */
- if (childerr != 0) {
+ /*
+ * If we couldn't invoke the utility or if utility exited because of a
+ * signal or with a value of 255, warn (per POSIX), and then wait until
+ * all other children have exited before exiting 1-125. POSIX requires
+ * xargs to stop reading if child exits because of a signal or * with
+ * 255, but it does not require us to exit immediately; waiting is
+ * preferable to orphaning.
+ */
+ if (childerr != 0 && cause_exit == 0) {
errno = childerr;
- err(errno == ENOENT ? 127 : 126, "%s", name);
- }
- if (WIFSIGNALED(status))
- errx(1, "%s: terminated with signal %d; aborting",
+ waitall = 1;
+ cause_exit = ENOENT ? 127 : 126;
+ warn("%s", name);
+ } else if (WIFSIGNALED(status)) {
+ waitall = cause_exit = 1;
+ warnx("%s: terminated with signal %d; aborting",
name, WTERMSIG(status));
- if (WEXITSTATUS(status) == 255)
- errx(1, "%s: exited with status 255; aborting", name);
- if (WEXITSTATUS(status))
- rval = 1;
+ } else if (WEXITSTATUS(status) == 255) {
+ waitall = cause_exit = 1;
+ warnx("%s: exited with status 255; aborting", name);
+ } else if (WEXITSTATUS(status))
+ rval = 1;
}
+ if (cause_exit)
+ exit(cause_exit);
if (pid == -1 && errno != ECHILD)
err(1, "waitpid");
}
More information about the freebsd-arch
mailing list