From 223ac53797d33b0473323efc0d5a44d1dceaf746 Mon Sep 17 00:00:00 2001 From: Peter Stephenson Date: Sun, 26 Oct 2014 17:47:42 +0000 Subject: [PATCH 1/2] 33531 with additions: retain status of exited background jobs. Add linked list of unwaited-for background jobs. Truncate at value of _SC_CHILD_MAX discarding oldest. Remove old lastpid_status mechanism for latest exited process only. Slightly tighten safety of permanently allocated linked lists so that this doesn't compromise signal handling. Upstream-commit: b4f7ccecd93ca9e64c3c3c774fdaefae83d7204a Signed-off-by: Kamil Dudka --- Doc/Zsh/builtins.yo | 16 ++++++ Doc/Zsh/options.yo | 8 +-- Doc/zshoptions.1 | 8 +-- Src/exec.c | 2 - Src/init.c | 1 - Src/jobs.c | 138 ++++++++++++++++++++++++++++++++++++++++++++-------- Src/linklist.c | 4 ++ Src/signals.c | 14 +++--- 8 files changed, 152 insertions(+), 39 deletions(-) diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo index 46f40cc..edc335e 100644 --- a/Doc/Zsh/builtins.yo +++ b/Doc/Zsh/builtins.yo @@ -1905,6 +1905,22 @@ then all currently active child processes are waited for. Each var(job) can be either a job specification or the process ID of a job in the job table. The exit status from this command is that of the job waited for. + +It is possible to wait for recent processes (specified by process ID, +not by job) that were running in the background even if the process has +exited. Typically the process ID will be recorded by capturing the +value of the variable tt($!) immediately after the process has been +started. There is a limit on the number of process IDs remembered by +the shell; this is given by the value of the system configuration +parameter tt(CHILD_MAX). When this limit is reached, older process IDs +are discarded, least recently started processes first. + +Note there is no protection against the process ID wrapping, i.e. if the +wait is not executed soon enough there is a chance the process waited +for is the wrong one. A conflict implies both process IDs have been +generated by the shell, as other processes are not recorded, and that +the user is potentially interested in both, so this problem is intrinsic +to process IDs. ) findex(whence) item(tt(whence) [ tt(-vcwfpams) ] var(name) ...)( diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo index 068a253..452b258 100644 --- a/Doc/Zsh/options.yo +++ b/Doc/Zsh/options.yo @@ -1401,10 +1401,10 @@ shell is saved for output within a subshell (for example, within a pipeline). When the option is set, the output of tt(jobs) is empty until a job is started within the subshell. -When the option is set, it becomes possible to use the tt(wait) builtin to -wait for the last job started in the background (as given by tt($!)) even -if that job has already exited. This works even if the option is turned -on temporarily around the use of the tt(wait) builtin. +In previous versions of the shell, it was necessary to enable +tt(POSIX_JOBS) in order for the builtin command tt(wait) to return the +status of background jobs that had already exited. This is no longer +the case. ) enditem() diff --git a/Doc/zshoptions.1 b/Doc/zshoptions.1 index dc8f2f8..17f9e97 100644 --- a/Doc/zshoptions.1 +++ b/Doc/zshoptions.1 @@ -867,10 +867,10 @@ shell is saved for output within a subshell (for example, within a pipeline)\&. When the option is set, the output of \fBjobs\fP is empty until a job is started within the subshell\&. .PP -When the option is set, it becomes possible to use the \fBwait\fP builtin to -wait for the last job started in the background (as given by \fB$!\fP) even -if that job has already exited\&. This works even if the option is turned -on temporarily around the use of the \fBwait\fP builtin\&. +In previous versions of the shell, it was necessary to enable +\fBPOSIX_JOBS\fP in order for the builtin command \fBwait\fP to return the +status of background jobs that had already exited\&. This is no longer +the case\&. .RE .RE .PP diff --git a/Src/exec.c b/Src/exec.c index d0fadd6..a9c4688 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -2852,8 +2852,6 @@ execcmd(Estate state, int input, int output, int how, int last1) #endif if (how & Z_ASYNC) { lastpid = (zlong) pid; - /* indicate it's possible to set status for lastpid */ - lastpid_status = -2L; } else if (!jobtab[thisjob].stty_in_env && varspc) { /* search for STTY=... */ Wordcode p = varspc; diff --git a/Src/init.c b/Src/init.c index c26d887..6666f98 100644 --- a/Src/init.c +++ b/Src/init.c @@ -1018,7 +1018,6 @@ setupvals(void) bufstack = znewlinklist(); hsubl = hsubr = NULL; lastpid = 0; - lastpid_status = -1L; get_usage(); diff --git a/Src/jobs.c b/Src/jobs.c index bd95afb..18bb648 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -104,15 +104,6 @@ int prev_errflag, prev_breaks, errbrk_saved; /**/ int numpipestats, pipestats[MAX_PIPESTATS]; -/* - * The status associated with the process lastpid. - * -1 if not set and no associated lastpid - * -2 if lastpid is set and status isn't yet - * else the value returned by wait(). - */ -/**/ -long lastpid_status; - /* Diff two timevals for elapsed-time computations */ /**/ @@ -1210,14 +1201,6 @@ addproc(pid_t pid, char *text, int aux, struct timeval *bgtime) { Process pn, *pnlist; - if (pid == lastpid && lastpid_status != -2L) { - /* - * The status for the previous lastpid is invalid. - * Presumably process numbers have wrapped. - */ - lastpid_status = -1L; - } - DPUTS(thisjob == -1, "No valid job in addproc."); pn = (Process) zshcalloc(sizeof *pn); pn->pid = pid; @@ -1826,6 +1809,122 @@ maybeshrinkjobtab(void) unqueue_signals(); } +/* + * Definitions for the background process stuff recorded below. + * This would be more efficient as a hash, but + * - that's quite heavyweight for something not needed very often + * - we need some kind of ordering as POSIX allows us to limit + * the size of the list to the value of _SC_CHILD_MAX and clearly + * we want to clear the oldest first + * - cases with a long list of background jobs where the user doesn't + * wait for a large number, and then does wait for one (the only + * inefficient case) are rare + * - in the context of waiting for an external process, looping + * over a list isn't so very inefficient. + * Enough excuses already. + */ + +/* Data in the link list, a key (process ID) / value (exit status) pair. */ +struct bgstatus { + pid_t pid; + int status; +}; +typedef struct bgstatus *Bgstatus; +/* The list of those entries */ +LinkList bgstatus_list; +/* Count of entries. Reaches value of _SC_CHILD_MAX and stops. */ +long bgstatus_count; + +/* + * Remove and free a bgstatus entry. + */ +static void rembgstatus(LinkNode node) +{ + zfree(remnode(bgstatus_list, node), sizeof(struct bgstatus)); + bgstatus_count--; +} + +/* + * Record the status of a background process that exited so we + * can execute the builtin wait for it. + * + * We can't execute the wait builtin for something that exited in the + * foreground as it's not visible to the user, so don't bother recording. + */ + +/**/ +void +addbgstatus(pid_t pid, int status) +{ + static long child_max; + Bgstatus bgstatus_entry; + + if (!child_max) { +#ifdef _SC_CHILD_MAX + child_max = sysconf(_SC_CHILD_MAX); + if (!child_max) /* paranoia */ +#endif + { + /* Be inventive */ + child_max = 1024L; + } + } + + if (!bgstatus_list) { + bgstatus_list = znewlinklist(); + /* + * We're not always robust about memory failures, but + * this is pretty deep in the shell basics to be failing owing + * to memory, and a failure to wait is reported loudly, so test + * and fail silently here. + */ + if (!bgstatus_list) + return; + } + if (bgstatus_count == child_max) { + /* Overflow. List is in order, remove first */ + rembgstatus(firstnode(bgstatus_list)); + } + bgstatus_entry = (Bgstatus)zalloc(sizeof(*bgstatus_entry)); + if (!bgstatus_entry) { + /* See note above */ + return; + } + bgstatus_entry->pid = pid; + bgstatus_entry->status = status; + if (!zaddlinknode(bgstatus_list, bgstatus_entry)) { + zfree(bgstatus_entry, sizeof(*bgstatus_entry)); + return; + } + bgstatus_count++; +} + +/* + * See if pid has a recorded exit status. + * Note we make no guarantee that the PIDs haven't wrapped, so this + * may not be the right process. + * + * This is only used by wait, which must only work on each + * pid once, so we need to remove the entry if we find it. + */ + +static int getbgstatus(pid_t pid) +{ + LinkNode node; + Bgstatus bgstatus_entry; + + if (!bgstatus_list) + return -1; + for (node = firstnode(bgstatus_list); node; incnode(node)) { + bgstatus_entry = (Bgstatus)getdata(node); + if (bgstatus_entry->pid == pid) { + int status = bgstatus_entry->status; + rembgstatus(node); + return status; + } + } + return -1; +} /* bg, disown, fg, jobs, wait: most of the job control commands are * * here. They all take the same type of argument. Exception: wait can * @@ -1971,10 +2070,7 @@ bin_fg(char *name, char **argv, Options ops, int func) } if (retval == 0) retval = lastval2; - } else if (isset(POSIXJOBS) && - pid == lastpid && lastpid_status >= 0L) { - retval = (int)lastpid_status; - } else { + } else if ((retval = getbgstatus(pid)) < 0) { zwarnnam(name, "pid %d is not a child of this shell", pid); /* presumably lastval2 doesn't tell us a heck of a lot? */ retval = 1; diff --git a/Src/linklist.c b/Src/linklist.c index 1e364fb..3aa8125 100644 --- a/Src/linklist.c +++ b/Src/linklist.c @@ -118,6 +118,8 @@ znewlinklist(void) LinkList list; list = (LinkList) zalloc(sizeof *list); + if (!list) + return NULL; list->list.first = NULL; list->list.last = &list->node; list->list.flags = 0; @@ -152,6 +154,8 @@ zinsertlinknode(LinkList list, LinkNode node, void *dat) tmp = node->next; node->next = new = (LinkNode) zalloc(sizeof *tmp); + if (!new) + return NULL; new->prev = node; new->dat = dat; new->next = tmp; diff --git a/Src/signals.c b/Src/signals.c index 2df69f9..e728505 100644 --- a/Src/signals.c +++ b/Src/signals.c @@ -520,14 +520,14 @@ wait_for_processes(void) get_usage(); } /* - * Remember the status associated with $!, so we can - * wait for it even if it's exited. This value is - * only used if we can't find the PID in the job table, - * so it doesn't matter that the value we save here isn't - * useful until the process has exited. + * Accumulate a list of older jobs. We only do this for + * background jobs, which is something in the job table + * that's not marked as in the current shell or as shell builtin + * and is not equal to the current foreground job. */ - if (pn != NULL && pid == lastpid && lastpid_status != -1L) - lastpid_status = lastval2; + if (jn && !(jn->stat & (STAT_CURSH|STAT_BUILTIN)) && + jn - jobtab != thisjob) + addbgstatus(pid, (int)lastval2); } } -- 2.1.0 From 2d59469450ba80b69449dc2777f0fc0673e0fbd6 Mon Sep 17 00:00:00 2001 From: Peter Stephenson Date: Sun, 26 Oct 2014 19:04:47 +0000 Subject: [PATCH 2/2] 33542: test logic for waiting for already exited processes Upstream-commit: 9a551ca85999ff329714fd2cca138ce2f7d3c3d9 Signed-off-by: Kamil Dudka --- Test/A05execution.ztst | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/Test/A05execution.ztst b/Test/A05execution.ztst index ca97f4f..589815f 100644 --- a/Test/A05execution.ztst +++ b/Test/A05execution.ztst @@ -178,3 +178,28 @@ kill $! 0:Status reset by starting a backgrounded command >0 + +# This tests that we record the status of processes that have already exited +# for when we wait for them. +# +# Actually, we don't guarantee here that the jobs have already exited, but +# the order of the waits means it's highly likely we do need to recall a +# previous status, barring accidents which shouldn't happen very often. In +# other words, we rely on the test working repeatedly rather than just +# once. The monitor option is irrelevant to the logic, so we'll make +# our job easier by turning it off. + unsetopt monitor + (exit 1) & + one=$! + (exit 2) & + two=$! + (exit 3) & + three=$! + wait $three + print $? + wait $two + print $? + wait $one +1:The status of recently exited background jobs is recorded +>3 +>2 -- 2.1.0