Skip site navigation (1)Skip section navigation (2)
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>