From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6064 invoked by alias); 24 Oct 2013 16:26:52 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 31885 Received: (qmail 19611 invoked from network); 24 Oct 2013 16:26:47 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 From: Bart Schaefer Message-id: <131024092633.ZM27623@torch.brasslantern.com> Date: Thu, 24 Oct 2013 09:26:33 -0700 In-reply-to: <20131024095713.44d2982e@pwslap01u.europe.root.pri> Comments: In reply to Peter Stephenson "Re: Strange function/pipestatus behavior, maybe a scope bug?" (Oct 24, 9:57am) References: <20131023092155.5ba8a54b@pwslap01u.europe.root.pri> <20131023221412.5cdecd76@pws-pc.ntlworld.com> <131023221548.ZM2352@torch.brasslantern.com> <20131024095713.44d2982e@pwslap01u.europe.root.pri> X-Mailer: OpenZMail Classic (0.9.2 24April2005) To: zsh-workers@zsh.org Subject: Re: Strange function/pipestatus behavior, maybe a scope bug? MIME-version: 1.0 Content-type: text/plain; charset=us-ascii On Oct 24, 9:57am, Peter Stephenson wrote: } Subject: Re: Strange function/pipestatus behavior, maybe a scope bug? } } On Wed, 23 Oct 2013 22:15:48 -0700 } Bart Schaefer wrote: } > something goes wrong and the whole thing is treated as if it is in } > the background. Here's zsh-5.0.2-248-ge05261f without my patch: } > } > % true | false | true | false | (){ true }; echo $pipestatus } > [1] + done true | } > exit 1 false | } > done true | } > running false } > 1 } > % } } Sounds like a race setting up the job, maybe related to the fact that } "thisjob" isn't initialised in a particularly obvious place when the } code is executing within the shell. Are you seeing this with any } shell function, or just anonymous functions? Any shell function will do it, except one that runs an external command. The odd thing is that this doesn't require "zsh -f", it happens for the first interactive command in a new shell even after loading the whole of compinit etc. It does NOT happen if the MONITOR option is not set. Here's a different clue: % true | false | (){ sleep 10; true } zsh: done true | zsh: exit 1 false | zsh: suspended () { ... } % echo $? : $pipestatus 20 : 0 1 0 % fg [1] + done true | exit 1 false | continued () { ... } (anon):fg: no job control in this shell. % This one is repeatable (happens every time, not just the first, and with any function, not just an anonymous one). } > Oh, one other bug which I don't believe is new: the PIPEFAIL option } > doesn't work right if the last command is a builtin. Maybe that } > test should be moved into storepipestats() as well, in which case } > the redundancy I mentioned above might also be eliminated. } } Are you saying that $pipestatus is correct but the overall status is } wrong if you have something like "true | true | true" or "true | true | } false"? Not exactly. PIPEFAIL means that if any command anywhere in the pipeline fails, then the entire pipeline fails. *Without* PIPEFAIL, the command true | (return 3) | true shoule have pipefail=(0 3 0) and exit status 0. *With* PIPEFAIL, it should have pipefail=(0 3 0) and exit status 3. But it does not, becuase "true" is a builtin. Replace with /bin/true and the PIPEFAIL exit status is correct. } It certainly sounds like they ought to run at the same point. Yeah, I'm becoming convinced that the PIPEFAIL tests also need to be moved into storepipestats(). It's a little messy that you can't tell from the Job pointer whether the job *number* is equal to thisjob. Since inforground can be true in update_job even when job != thisjob, but PIPEFAIL only applies when job == thisjob, it probably means passing an extra parameter. Having implemented this, I can't force a case where it's the right thing to call storepipestats() from update_job(). On the other hand, I'm also getting signal handling race conditions again, particularly when sending TSTP (^Z) to a pipeline that ends with (){ sleep N } -- the value of pipestatus is correct, but the job gets treated as done when it should be stopped, so the stopped sleep is orphaned. Somebody else please try this patch and see if it works consistently for you. Aside: I did think of another reason we might want to keep a copy of pipestatus in the Job object: so that we can restore it when a job that has been stopped or backgrounded is brought into the foreground again. Right now the exit status of "fg" seems to always be zero even after suspending/resuming (){ sleep 10; false }, and pipestatus is only valid right after suspending the job, not upon resuming it. diff --git a/Src/jobs.c b/Src/jobs.c index 82ffdf2..c218743 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -377,8 +377,8 @@ check_cursh_sig(int sig) } /**/ -int -storepipestats(Job jn, int inforeground) +void +storepipestats(Job jn, int inforeground, int fixlastval) { int i, pipefail = 0, jpipestats[MAX_PIPESTATS]; Process p; @@ -397,7 +397,13 @@ storepipestats(Job jn, int inforeground) numpipestats = i; } - return pipefail; + if (fixlastval) { + if (jn->stat & STAT_CURSH) { + if (!lastval && isset(PIPEFAIL)) + lastval = pipefail; + } else if (isset(PIPEFAIL)) + lastval = pipefail; + } } /* Update status of job, possibly printing it */ @@ -532,15 +538,12 @@ update_job(Job jn) jn->stat |= (somestopped) ? STAT_CHANGED | STAT_STOPPED : STAT_CHANGED | STAT_DONE; if (jn->stat & STAT_DONE) { - int newlastval = storepipestats(jn, inforeground); - - if (job == thisjob) { - if (jn->stat & STAT_CURSH) { - if (!lastval && isset(PIPEFAIL)) - lastval = newlastval; - } else if (isset(PIPEFAIL)) - lastval = newlastval; - } + /* This may be redundant with printjob() but note that inforeground + * is true here for STAT_CURSH jobs even when job != thisjob, most + * likely because thisjob = -1 from exec.c:execsimple() trickery. + * However, if we reset lastval here we break it for printjob(). + */ + storepipestats(jn, inforeground, 0); } if (!inforeground && (jn->stat & (STAT_SUBJOB | STAT_DONE)) == (STAT_SUBJOB | STAT_DONE)) { @@ -991,7 +994,8 @@ printjob(Job jn, int lng, int synch) if (skip_print) { if (jn->stat & STAT_DONE) { - (void) storepipestats(jn, job == thisjob); + /* This looks silly, but see update_job() */ + storepipestats(jn, job == thisjob, job == thisjob); if (should_report_time(jn)) dumptime(jn); deletejob(jn, 0); @@ -1107,9 +1111,9 @@ printjob(Job jn, int lng, int synch) fflush(fout); } -/* print "(pwd now: foo)" messages: with (lng & 4) we are printing - * the directory where the job is running, otherwise the current directory - */ + /* print "(pwd now: foo)" messages: with (lng & 4) we are printing + * the directory where the job is running, otherwise the current directory + */ if ((lng & 4) || (interact && job == thisjob && jn->pwd && strcmp(jn->pwd, pwd))) { @@ -1119,10 +1123,12 @@ printjob(Job jn, int lng, int synch) fprintf(fout, ")\n"); fflush(fout); } -/* delete job if done */ + + /* delete job if done */ if (jn->stat & STAT_DONE) { - (void) storepipestats(jn, job == thisjob); + /* This looks silly, but see update_job() */ + storepipestats(jn, job == thisjob, job == thisjob); if (should_report_time(jn)) dumptime(jn); deletejob(jn, 0);