Change to xargs to avoid orphaning of utility processes on
signal|exit 255 from child
Matthew Story
matthewstory at gmail.com
Sat Mar 10 04:27:25 UTC 2012
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
This patch includes the cleaned-up changes Jilles sent back earlier (thanks
again).
--
regards,
matt
-------------- next part --------------
Index: xargs.1
===================================================================
--- xargs.1 (revision 232764)
+++ 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 232764)
+++ 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);
@@ -308,8 +309,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 +345,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,8 +365,10 @@
*/
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;
@@ -394,8 +401,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 +413,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 +462,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 +492,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 +566,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 +613,47 @@
}
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) {
errno = childerr;
- err(errno == ENOENT ? 127 : 126, "%s", name);
+ waitall = 1;
+ cause_exit = ENOENT ? 127 : 126;
+ warn("%s", name);
}
- if (WIFSIGNALED(status))
- errx(1, "%s: terminated with signal %d; aborting",
+ /*
+ * 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 (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