Date: Sat, 10 Mar 2012 13:23:02 -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%2B9ogedvEX-Au4Qw_sVczxOYB70yuYiiY8jh1i5gxq1W7rOXQ@mail.gmail.com> In-Reply-To: <CAB%2B9ogev5ksni%2Bfab_kFWyak_2HxNiaGpyaQnRMDr6Yduea_ig@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> <CAB%2B9ogev5ksni%2Bfab_kFWyak_2HxNiaGpyaQnRMDr6Yduea_ig@mail.gmail.com>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] On Fri, Mar 9, 2012 at 11:27 PM, Matthew Story <matthewstory@gmail.com>wrote: > 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 > 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. > > This patch includes the cleaned-up changes Jilles sent back earlier > (thanks again). > > -- > regards, > matt > -- regards, matt [-- Attachment #2 --] Index: xargs.1 =================================================================== --- xargs.1 (revision 232787) +++ 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 232787) +++ 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,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) { + /* If we couldn't invoke the utility, warn and flag for exit. */ + if (childerr != 0 && cause_exit == 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"); }help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAB%2B9ogedvEX-Au4Qw_sVczxOYB70yuYiiY8jh1i5gxq1W7rOXQ>