Date: Fri, 9 Mar 2012 23:27:24 -0500 From: Matthew Story <matthewstory@gmail.com> To: freebsd-arch@freebsd.org Cc: Jilles Tjoelker <jilles@stack.nl> Subject: Re: Change to xargs to avoid orphaning of utility processes on signal|exit 255 from child Message-ID: <CAB%2B9ogev5ksni%2Bfab_kFWyak_2HxNiaGpyaQnRMDr6Yduea_ig@mail.gmail.com> In-Reply-To: <CAB%2B9ogdEUa3Q=jBDX-JeX1S7O_95E8MVc8XK2F7y8P0zcTaqiQ@mail.gmail.com> References: <CAB%2B9ogetrht1Ttf53vSbq7L5Fe9DbB5igSynKM3=tjZh0z0_ew@mail.gmail.com> <CAB%2B9ogcVr-pNof-Tncc6F8CKjm3suqt=U-zvy9We0YEouwxhFw@mail.gmail.com> <20120309225433.GA32725@stack.nl> <CAB%2B9ogdEUa3Q=jBDX-JeX1S7O_95E8MVc8XK2F7y8P0zcTaqiQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Fri, Mar 9, 2012 at 6:13 PM, Matthew Story <matthewstory@gmail.com>wrote: > On Fri, Mar 9, 2012 at 5:54 PM, Jilles Tjoelker <jilles@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@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 [-- Attachment #2 --] 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"); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAB%2B9ogev5ksni%2Bfab_kFWyak_2HxNiaGpyaQnRMDr6Yduea_ig>