From 223ac53797d33b0473323efc0d5a44d1dceaf746 Mon Sep 17 00:00:00 2001

From: Peter Stephenson <p.w.stephenson@ntlworld.com>

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 <kdudka@redhat.com>

---

 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 <p.w.stephenson@ntlworld.com>

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 <kdudka@redhat.com>

---

 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