From owner-freebsd-arch@FreeBSD.ORG Fri Mar 9 23:13:30 2012 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2AB99106564A for ; Fri, 9 Mar 2012 23:13:30 +0000 (UTC) (envelope-from matthewstory@gmail.com) Received: from mail-vx0-f182.google.com (mail-vx0-f182.google.com [209.85.220.182]) by mx1.freebsd.org (Postfix) with ESMTP id CCC528FC0A for ; Fri, 9 Mar 2012 23:13:29 +0000 (UTC) Received: by vcmm1 with SMTP id m1so2325815vcm.13 for ; Fri, 09 Mar 2012 15:13:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=Nhd9oRfV6e4UmBDHkNlIkuGeicPH7/vU4xw+V3vBUgI=; b=j4Jzhvr+EtI9/6u4rTp2+4Y4/hmmf+c50qR8rwZJpP6ViE4JphvnR0Pd1dqLQbmBVm VK+5RPpkH2vQavsRgNTeExt3fvASq3UZZkKA8vX9xkPtpnA4nOC62iAmuYqvFLKGsg8/ gI408OPlOywpQNr/zWxmBXO2VxA1bmdTxqi/5Coi8epAmYDLrzei517Gx/x/q8GLJBoA bbsLfe0lLBLNZrnvzkB5BI66fRIKGtUidimhwm0NOb3BcTPWdLX8kg/r93CwmXphC6ir EJlT+Q4xRr3N9dADABOUkOOF6JqCFJ5fnj5RoyN0zVWZrf3PXsBwB6GUC5yo/49forhd ZJzw== MIME-Version: 1.0 Received: by 10.52.27.111 with SMTP id s15mr6516887vdg.120.1331334803519; Fri, 09 Mar 2012 15:13:23 -0800 (PST) Received: by 10.52.93.42 with HTTP; Fri, 9 Mar 2012 15:13:23 -0800 (PST) In-Reply-To: <20120309225433.GA32725@stack.nl> References: <20120309225433.GA32725@stack.nl> Date: Fri, 9 Mar 2012 18:13:23 -0500 Message-ID: From: Matthew Story To: freebsd-arch@freebsd.org Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.5 Cc: Jilles Tjoelker Subject: Re: Change to xargs to avoid orphaning of utility processes on signal|exit 255 from child X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Mar 2012 23:13:30 -0000 On Fri, Mar 9, 2012 at 5:54 PM, Jilles Tjoelker 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 >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