From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: zsh-workers-return-43409-ml=inbox.vuxu.org@zsh.org X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE,T_DKIMWL_WL_HIGH autolearn=ham autolearn_force=no version=3.4.1 Received: from primenet.com.au (ns1.primenet.com.au [203.24.36.2]) by inbox.vuxu.org (OpenSMTPD) with ESMTP id b8298c64 for ; Fri, 7 Sep 2018 14:58:46 +0000 (UTC) Received: (qmail 2392 invoked by alias); 7 Sep 2018 14:58:35 -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: List-Unsubscribe: X-Seq: 43409 Received: (qmail 7103 invoked by uid 1010); 7 Sep 2018 14:58:35 -0000 X-Qmail-Scanner-Diagnostics: from mailout2.w1.samsung.com by f.primenet.com.au (envelope-from , uid 7791) with qmail-scanner-2.11 (clamdscan: 0.99.2/21882. spamassassin: 3.4.1. Clear:RC:0(210.118.77.12):SA:0(-6.9/5.0):. Processed in 1.044015 secs); 07 Sep 2018 14:58:35 -0000 X-Envelope-From: p.stephenson@samsung.com X-Qmail-Scanner-Mime-Attachments: | X-Qmail-Scanner-Zip-Files: | DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20180907145830euoutp0238aefc6d9b2c82dba74895b0200917b1~SJhieo-Mn0074000740euoutp02c DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1536332310; bh=5N1fGgFa7+Zl0rJ263D3v4877943oaW7oxtesJ3nezo=; h=Date:From:To:Subject:In-Reply-To:References:From; b=r61pUJsGpwXOyitbEXCnluIpk6nl/pnyOsjcDi7Y4u9qo8/NJYNNAaHZ+xHrQB1Gb y//Pv3QX+BDM3Pmb00pahjMT2RRNw1SKAvtxar9tAS/QcvMXa5ViVXoweRJJEzZhQi X3+iIHs0SiBc/hRjChR/oj38uoWxL5qXfSf9UXi8= X-AuditID: cbfec7f5-34dff700000012c6-79-5b929214c730 Date: Fri, 7 Sep 2018 15:58:26 +0100 From: Peter Stephenson To: Zsh hackers list Subject: Re: PATCH: Rimmerworld pipeline race In-Reply-To: <20180907150140.46a05880@camnpupstephen.cam.scsc.local> Organization: SCSC X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrLIsWRmVeSWpSXmKPExsWy7djP87qikyZFG8xfyGRxsPkhkwOjx6qD H5gCGKO4bFJSczLLUov07RK4Mu4tOsxUcNW84sGFA8wNjO1aXYycHBICJhL3P35k7GLk4hAS WMEoMWPxcXYIp49JYsaVfiYIp5dJ4tq+bewwLdO7u6GqljNK3LnSzQqSAKvqWi8MkTjNKHHo 7Dc2iMR5Romm1y5djBwcLAIqEp2zI0HCbAKGElM3zWYEsUUEtCR2nDzJBGILC+hKrPz5DizO KeAi0b9oERtIK7+AkMSFZluIG+wlju6BKOcVEJQ4OfMJC4jNLCAvsf3tHGaQEyQEPrNJ/D27 GqqoTGJGy2pGiGYXicaXa9kgbGGJV8e3QD0mI/F/53wmiOZ2Rok1k16zQzg9jBKbjt6B6raW 6Lt9kRHkImYBTYn1u/Qhwo4SN18eYwEJSwjwSdx4KwhxEJ/EpG3TmSHCvBIdbUIQ1WoSO5q2 Mk5gVJ6F5IVZSF6YhTB/ASPzKkbx1NLi3PTUYuO81HK94sTc4tK8dL3k/NxNjMBEcPrf8a87 GPf9STrEKMDBqMTDG1A2KVqINbGsuDL3EKMEB7OSCK9rHVCINyWxsiq1KD++qDQntfgQozQH i5I4L59WWrSQQHpiSWp2ampBahFMlomDU6qBsdRGOTR8ywzX1v7fp6uDRNy+O62IOWD2nfmC 5NJFBrpttyby6u25wpa6L2TdCd8Yyy2TG98fuOkf6SDOG7pdb7Gls+XxCf83Si/c8++u2Vrh rONL2fLdtI46S2U/3amh4vHxjYyeg2nSDYV0/mB+L4XIew8XGi3R0pXvX74w7Y1Gj///6fNu KrEUZyQaajEXFScCAK0r6EMAAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFuphkeLIzCtJLcpLzFFi42I5/e/4PV2RSZOiDfaf4LQ42PyQyYHRY9XB D0wBjFF6NkX5pSWpChn5xSW2StGGFkZ6hpYWekYmlnqGxuaxVkamSvp2NimpOZllqUX6dgl6 GfcWHWYquGpe8eDCAeYGxnatLkZODgkBE4np3d3sXYxcHEICSxklzvz7yQiRkJH4dOUjO4Qt LPHnWhcbiC0k0M0kMWdLMETDaUaJaXv/sEI45xklruz7DNTNwcEioCLROTsSpIFNwFBi6qbZ YENFBLQkdpw8yQRiCwvoSqz8+Q4szivgLHF4fSPYMk4BF4n+RYvYIGauYJG4eq2bFWQmv4CQ xIVmW4iD7CWO7oGYwysgKHFy5hMWEJtZQEfixKpjzBC2vMT2t3OYJzAKz0JSNgtJ2SwkZQsY mVcxiqSWFuem5xYb6hUn5haX5qXrJefnbmIExsS2Yz8372C8tDH4EKMAB6MSD29A2aRoIdbE suLK3EOMEhzMSiK8rnVAId6UxMqq1KL8+KLSnNTiQ4ymwLCYyCwlmpwPjNe8knhDU0NzC0tD c2NzYzMLJXHe8waVUUIC6YklqdmpqQWpRTB9TBycUg2MUbc3q736d+O63r0XHU7KRdlW1iH6 crv1u3YuuL0yp/y8ZxKnWkxQ53Etpze7LNc/vFTXfK6M62QH14VHb+479nBItLOcX5rz++uk FIYDTT7Bf5U86lkTvz1Pt13UdGoNv+/PKftc67RK5S1MhL5vsP2t1f1tdtbukh2M6q+3KOcw fz/66tAiJZbijERDLeai4kQAZOCRXp8CAAA= Message-Id: <20180907145828eucas1p2755db3a0eb9725cb3a29121033e77f21~SJhg-Al9h2728227282eucas1p2z@eucas1p2.samsung.com> X-CMS-MailID: 20180907145828eucas1p2755db3a0eb9725cb3a29121033e77f21 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20180905200816epcas5p18ce6c49baa637e7d83a769e97c4364fb X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20180905200816epcas5p18ce6c49baa637e7d83a769e97c4364fb References: <20180905210740.5a6aec15@pws-HP.localdomain> <20180906090902.1f344e9f@camnpupstephen.cam.scsc.local> <20180906092250eucas1p13d651e07ae627d179dd0701e65f912d6~RxTLztJrP2263722637eucas1p12@eucas1p1.samsung.com> <20180907101852.62415ff9@camnpupstephen.cam.scsc.local> <20180907122145.2af5bcba@camnpupstephen.cam.scsc.local> <20180907150140.46a05880@camnpupstephen.cam.scsc.local> On Fri, 7 Sep 2018 15:01:40 +0100 Peter Stephenson wrote: > OK, this is *much* closer and possibly correct. Here's a more sophisticated fix, though I'll take votes on which to use for now. Use the existing subshell syncronisation mechanism to return the group leader actually used by the subshell, if it picks one. No second guessing involved --- just argument passing and a read/write between the processes that was already there but used just to terminate in an error when the write side was closed. pws diff --git a/Src/exec.c b/Src/exec.c index 09ee132..074265f 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -994,9 +994,15 @@ enum { ESUB_JOB_CONTROL = 0x40 }; +/* + * gleaderp may be NULL. Otherwise, *gleaderp is set to point to the + * group leader of the job of the new process if this is assigned. Else + * it is left alone: it is initialised to -1. + */ + /**/ static void -entersubsh(int flags) +entersubsh(int flags, int *gleaderp) { int i, sig, monitor, job_control_ok; @@ -1030,6 +1036,8 @@ entersubsh(int flags) if (!(flags & ESUB_ASYNC)) attachtty(jobtab[thisjob].gleader); } + if (gleaderp) + *gleaderp = jobtab[list_pipe_job].gleader; } else if (!jobtab[thisjob].gleader || setpgrp(0L, jobtab[thisjob].gleader) == -1) { @@ -1050,6 +1058,8 @@ entersubsh(int flags) setpgrp(0L, jobtab[thisjob].gleader); if (!(flags & ESUB_ASYNC)) attachtty(jobtab[thisjob].gleader); + if (gleaderp) + *gleaderp = jobtab[thisjob].gleader; } } if (!(flags & ESUB_FAKE)) @@ -1682,7 +1692,7 @@ execpline(Estate state, wordcode slcode, int how, int last1) curjob = newjob; DPUTS(!list_pipe_pid, "invalid list_pipe_pid"); addproc(list_pipe_pid, list_pipe_text, 0, - &list_pipe_start); + &list_pipe_start, -1); /* If the super-job contains only the sub-shell, the sub-shell is the group leader. */ @@ -1818,7 +1828,7 @@ execpline(Estate state, wordcode slcode, int how, int last1) } else { close(synch[0]); - entersubsh(ESUB_ASYNC); + entersubsh(ESUB_ASYNC, NULL); /* * At this point, we used to attach this process * to the process group of list_pipe_job (the @@ -2175,7 +2185,7 @@ closemn(struct multio **mfds, int fd, int type) } mn->ct = 1; mn->fds[0] = fd; - addproc(pid, NULL, 1, &bgtime); + addproc(pid, NULL, 1, &bgtime, -1); child_unblock(); return; } @@ -2676,7 +2686,7 @@ execcmd_fork(Estate state, int how, int type, Wordcode varspc, { pid_t pid; int synch[2], flags; - char dummy; + int gleader = -1; struct timeval bgtime; child_block(); @@ -2693,7 +2703,7 @@ execcmd_fork(Estate state, int how, int type, Wordcode varspc, } if (pid) { close(synch[1]); - read_loop(synch[0], &dummy, 1); + read_loop(synch[0], (char *)&gleader, sizeof(gleader)); close(synch[0]); if (how & Z_ASYNC) { lastpid = (zlong) pid; @@ -2711,7 +2721,7 @@ execcmd_fork(Estate state, int how, int type, Wordcode varspc, 3 : WC_ASSIGN_NUM(ac) + 2); } } - addproc(pid, text, 0, &bgtime); + addproc(pid, text, 0, &bgtime, gleader); if (oautocont >= 0) opts[AUTOCONTINUE] = oautocont; pipecleanfilelist(jobtab[thisjob].filelist, 1); @@ -2726,7 +2736,8 @@ execcmd_fork(Estate state, int how, int type, Wordcode varspc, if (type == WC_SUBSH && !(how & Z_ASYNC)) flags |= ESUB_JOB_CONTROL; *filelistp = jobtab[thisjob].filelist; - entersubsh(flags); + entersubsh(flags, &gleader); + write(synch[1], &gleader, sizeof(gleader)); close(synch[1]); zclose(close_if_forked); @@ -3842,7 +3853,7 @@ execcmd_exec(Estate state, Execcmd_params eparams, if ((do_exec || (type >= WC_CURSH && last1 == 1)) && !forked) flags |= ESUB_REVERTPGRP; - entersubsh(flags); + entersubsh(flags, NULL); } if (type == WC_FUNCDEF) { Eprog redir_prog; @@ -4604,7 +4615,7 @@ getoutput(char *cmd, int qt) child_unblock(); zclose(pipes[0]); redup(pipes[1], 1); - entersubsh(ESUB_PGRP|ESUB_NOMONITOR); + entersubsh(ESUB_PGRP|ESUB_NOMONITOR, NULL); cmdpush(CS_CMDSUBST); execode(prog, 0, 1, "cmdsubst"); cmdpop(); @@ -4798,7 +4809,7 @@ getoutputfile(char *cmd, char **eptr) /* pid == 0 */ redup(fd, 1); - entersubsh(ESUB_PGRP|ESUB_NOMONITOR); + entersubsh(ESUB_PGRP|ESUB_NOMONITOR, NULL); cmdpush(CS_CMDSUBST); execode(prog, 0, 1, "equalsubst"); cmdpop(); @@ -4865,7 +4876,7 @@ getproc(char *cmd, char **eptr) if (pid == -1) return NULL; if (!out) - addproc(pid, NULL, 1, &bgtime); + addproc(pid, NULL, 1, &bgtime, -1); procsubstpid = pid; return pnam; } @@ -4875,7 +4886,7 @@ getproc(char *cmd, char **eptr) zerr("can't open %s: %e", pnam, errno); _exit(1); } - entersubsh(ESUB_ASYNC|ESUB_PGRP); + entersubsh(ESUB_ASYNC|ESUB_PGRP, NULL); redup(fd, out); #else /* PATH_DEV_FD */ int pipes[2], fd; @@ -4902,12 +4913,12 @@ getproc(char *cmd, char **eptr) addfilelist(NULL, fd); if (!out) { - addproc(pid, NULL, 1, &bgtime); + addproc(pid, NULL, 1, &bgtime, -1); } procsubstpid = pid; return pnam; } - entersubsh(ESUB_ASYNC|ESUB_PGRP); + entersubsh(ESUB_ASYNC|ESUB_PGRP, NULL); redup(pipes[out], out); closem(FDT_UNUSED, 0); /* this closes pipes[!out] as well */ #endif /* PATH_DEV_FD */ @@ -4954,11 +4965,11 @@ getpipe(char *cmd, int nullexec) return -1; } if (!nullexec) - addproc(pid, NULL, 1, &bgtime); + addproc(pid, NULL, 1, &bgtime, -1); procsubstpid = pid; return pipes[!out]; } - entersubsh(ESUB_PGRP); + entersubsh(ESUB_PGRP, NULL); redup(pipes[out], out); closem(FDT_UNUSED, 0); /* this closes pipes[!out] as well */ cmdpush(CS_CMDSUBST); diff --git a/Src/jobs.c b/Src/jobs.c index 38b3d89..ba87a17 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -1375,7 +1375,7 @@ deletejob(Job jn, int disowning) /**/ void -addproc(pid_t pid, char *text, int aux, struct timeval *bgtime) +addproc(pid_t pid, char *text, int aux, struct timeval *bgtime, int gleader) { Process pn, *pnlist; @@ -1392,10 +1392,15 @@ addproc(pid_t pid, char *text, int aux, struct timeval *bgtime) if (!aux) { pn->bgtime = *bgtime; - /* if this is the first process we are adding to * - * the job, then it's the group leader. */ + /* + * if this is the first process we are adding to + * the job, then it's the group leader. + * + * Exception: if the forked subshell reported its own group + * leader, set that. + */ if (!jobtab[thisjob].gleader) - jobtab[thisjob].gleader = pid; + jobtab[thisjob].gleader = (gleader != -1) ? gleader : pid; /* attach this process to end of process list of current job */ pnlist = &jobtab[thisjob].procs; }