Date: Fri, 9 Mar 2012 18:13:23 -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%2B9ogdEUa3Q=jBDX-JeX1S7O_95E8MVc8XK2F7y8P0zcTaqiQ@mail.gmail.com> In-Reply-To: <20120309225433.GA32725@stack.nl> References: <CAB%2B9ogetrht1Ttf53vSbq7L5Fe9DbB5igSynKM3=tjZh0z0_ew@mail.gmail.com> <CAB%2B9ogcVr-pNof-Tncc6F8CKjm3suqt=U-zvy9We0YEouwxhFw@mail.gmail.com> <20120309225433.GA32725@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
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: > > > > > http://axe0.blackskyresearch.net/patches/matt/xargs.no_orphan.patch.txt--this version will apply to current xargs, and adds diagnostic > > > information for exit 255|signal to utility, as required by POSIX (see > > > PR165155). > > > > > > > http://axe0.blackskyresearch.net/patches/matt/xargs.no_orphan.PR165155.patch.txt--this version will apply on top of the patch in PR165155, as the errx > > > calls in that patch need to be modified to warnx calls. > > > I have updated these 2 patches above to branch correctly (the change from > > errx to warnx requires else'ing ... may have to purge browser catch to > pick > > up the change), wondering if this patch should be expanded to include all > > of the cases mentioned in the ``Consequences of Errors'' section of the > > POSIX specification: > > > > [... snip] > > > If a command line meeting the specified requirements cannot be > assembled, > > > the utility cannot be invoked, an invocation of the utility is > terminated > > > by a signal, or an invocation of the utility exits with exit status > 255, > > > the *xargs* utility shall write a diagnostic message and exit without > > > processing any remaining input. > > > 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: > > Index: xargs.1 > =================================================================== > --- xargs.1 (revision 232399) > +++ xargs.1 (working copy) > @@ -297,14 +297,18 @@ > The > .Nm > utility exits immediately (without processing any further input) if a > -command line cannot be assembled, > +command line cannot be assembled, or > .Ar utility > -cannot be invoked, an invocation of > +cannot be invoked. If 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 232399) > +++ xargs.c (working copy) > @@ -597,6 +597,7 @@ > { > 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. */ > @@ -604,15 +605,27 @@ > errno = childerr; > err(errno == ENOENT ? 127 : 126, "%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)) > + } 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(1); > if (pid == -1 && errno != ECHILD) > err(1, "waitpid"); > } > 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. -- regards, matt
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAB%2B9ogdEUa3Q=jBDX-JeX1S7O_95E8MVc8XK2F7y8P0zcTaqiQ>