From: Dominique Martinet <[email protected]>
'wait -n' had (at least) a couple of bugs:
1/ Waiting for jobs with non-zero status
if waiting for a command that returns non-zero, dowait() would keep
waiting for the next command.
That means something like the following waits 2 seconds instead of 1:
time busybox sh -c 'sh -c "sleep 1; false" & sleep 2 & wait -n'
2/ Waiting for jobs with multiple processes
While fixing the above, I noticed this comment:
/* wait -n waits for one _job_, not one _process_.
* date; sleep 3 & sleep 2 | sleep 1 & wait -n; date
* should wait for 2 seconds. Not 1 or 3.
*/
It turns out, that does not work:
time busybox sh -c 'sleep 3 & sleep 2 | sleep 1 & wait -n'
The current code returns -1 if a job is not finished, and dowait() stops
looping immediately on -1 since commit 8d5f465a206e ("ash: jobs: Fix
infinite loop in waitproc"), breaking this behaviour.
A test was added testing both of these cases, outputing the following
errors before patch:
```
Test1 (wait with zero exit status of bg process)
Test2 (wait with non-zero exit status of bg process)
Background1: BUG!
Test2 exit code was not 1: 0
Test3 (pipeline wait for whole job)
Test3 exit code was not 0: 129
Test3 finished in less than 2s
sh: can't kill pid 440510: No such process
Ok:1
```
This patch splits dowait() into two functions because the variant that
wants the pid is different enough to benefit from specialization.
function old new delta
waitone - 449 +449
waitcmd 277 340 +63
dowait 608 80 -528
------------------------------------------------------------------------------
(add/remove: 1/0 grow/shrink: 1/1 up/down: 512/-528) Total: -16 bytes
text data bss dec hex filename
786254 14292 1800 802346 c3e2a busybox_old
786238 14292 1800 802330 c3e1a busybox_unstripped
Fixes: 8d5f465a206e ("ash: jobs: Fix infinite loop in waitproc")
Tested-by: Wolfgang Zekoll <[email protected]>
Signed-off-by: Dominique Martinet <[email protected]>
---
v1 -> v2:
- fix test wording/add tested-by
v2 -> v3:
- fix breaking out of wait if a signal comes in,
and add wait9 test to check this keeps working.
Hopefully won't be more like this!
shell/ash.c | 61 +++++++++++++++--------------
shell/ash_test/ash-misc/wait8.right | 4 ++
shell/ash_test/ash-misc/wait8.tests | 47 ++++++++++++++++++++++
shell/ash_test/ash-misc/wait9.right | 6 +++
shell/ash_test/ash-misc/wait9.tests | 21 ++++++++++
5 files changed, 110 insertions(+), 29 deletions(-)
create mode 100644 shell/ash_test/ash-misc/wait8.right
create mode 100755 shell/ash_test/ash-misc/wait8.tests
create mode 100644 shell/ash_test/ash-misc/wait9.right
create mode 100644 shell/ash_test/ash-misc/wait9.tests
diff --git a/shell/ash.c b/shell/ash.c
index 841ffe8808fc..ee2d5c47c8b4 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -4418,9 +4418,6 @@ sprint_status48(char *os, int status, int sigonly)
#define DOWAIT_NONBLOCK 0 /* waitpid() will use WNOHANG and won't wait
for signals */
#define DOWAIT_BLOCK 1 /* waitpid() will NOT use WNOHANG */
#define DOWAIT_CHILD_OR_SIG 2 /* waitpid() will use WNOHANG and if got 0,
will wait for signals, then loop back */
-#if BASH_WAIT_N
-# define DOWAIT_JOBSTATUS 0x10 /* OR this to get job's exitstatus instead of
pid */
-#endif
static int
waitproc(int block, int *status)
@@ -4472,10 +4469,6 @@ static int waitone(int block, struct job *job)
int status;
struct job *jp;
struct job *thisjob = NULL;
-#if BASH_WAIT_N
- bool want_jobexitstatus = (block & DOWAIT_JOBSTATUS);
- block = (block & ~DOWAIT_JOBSTATUS);
-#endif
TRACE(("dowait(0x%x) called\n", block));
@@ -4501,7 +4494,7 @@ static int waitone(int block, struct job *job)
pid = waitproc(block, &status);
TRACE(("wait returns pid %d, status=%d\n", pid, status));
if (pid <= 0)
- goto out;
+ return pid;
for (jp = curjob; jp; jp = jp->prev_job) {
int jobstate;
@@ -4554,13 +4547,6 @@ static int waitone(int block, struct job *job)
out:
INTON;
-#if BASH_WAIT_N
- if (want_jobexitstatus) {
- pid = -1;
- if (thisjob && thisjob->state == JOBDONE)
- pid = thisjob->ps[thisjob->nprocs - 1].ps_status;
- }
-#endif
if (thisjob && thisjob == job) {
char s[48 + 1];
int len;
@@ -4572,32 +4558,49 @@ static int waitone(int block, struct job *job)
out2str(s);
}
}
- return pid;
+
+ status = INT_MAX;
+ if (thisjob && thisjob->state == JOBDONE)
+ status = thisjob->ps[thisjob->nprocs - 1].ps_status;
+
+ return status;
}
-static int dowait(int block, struct job *jp)
+static int dowait_status(void)
+{
+ int status, rstatus = -ENOENT;
+ int block = DOWAIT_CHILD_OR_SIG;
+
+ do {
+ status = waitone(block, NULL);
+ if (status >= 0 && status < INT_MAX) {
+ rstatus = status;
+ block = DOWAIT_NONBLOCK;
+ }
+ if (pending_sig)
+ break;
+ } while (status >= 0);
+
+ return rstatus;
+}
+
+static void dowait(int block, struct job *jp)
{
smallint gotchld = *(volatile smallint *)&gotsigchld;
- int rpid;
- int pid;
+ int status;
if (jp && jp->state != JOBRUNNING)
block = DOWAIT_NONBLOCK;
if (block == DOWAIT_NONBLOCK && !gotchld)
- return 1;
-
- rpid = 1;
+ return;
do {
- pid = waitone(block, jp);
- rpid &= !!pid;
+ status = waitone(block, jp);
- if (!pid || (jp && jp->state != JOBRUNNING))
+ if (!status || (jp && jp->state != JOBRUNNING))
block = DOWAIT_NONBLOCK;
- } while (pid >= 0);
-
- return rpid;
+ } while (status >= 0);
}
#if JOBS
@@ -4801,7 +4804,7 @@ waitcmd(int argc UNUSED_PARAM, char **argv)
* the trap is executed."
*/
#if BASH_WAIT_N
- status = dowait(DOWAIT_CHILD_OR_SIG | DOWAIT_JOBSTATUS,
NULL);
+ status = dowait_status();
#else
dowait(DOWAIT_CHILD_OR_SIG, NULL);
#endif
diff --git a/shell/ash_test/ash-misc/wait8.right
b/shell/ash_test/ash-misc/wait8.right
new file mode 100644
index 000000000000..5d29b8cc7dbf
--- /dev/null
+++ b/shell/ash_test/ash-misc/wait8.right
@@ -0,0 +1,4 @@
+Test1 (wait with zero exit status of bg process)
+Test2 (wait with non-zero exit status of bg process)
+Test3 (pipeline wait for whole job)
+Ok:0
diff --git a/shell/ash_test/ash-misc/wait8.tests
b/shell/ash_test/ash-misc/wait8.tests
new file mode 100755
index 000000000000..d29490260001
--- /dev/null
+++ b/shell/ash_test/ash-misc/wait8.tests
@@ -0,0 +1,47 @@
+# wait -n tests
+
+# should never wait for this one
+sleep 5 && echo "Background1: BUG!" &
+bg=$!
+
+# normal process
+echo "Test1 (wait with zero exit status of bg process)"
+sleep 1 &
+wait -n
+rc=$?
+if [ "$rc" != 0 ]; then
+ echo "Test1 exit code was not 0: $rc"
+fi
+
+# non-zero return code
+echo "Test2 (wait with non-zero exit status of bg process)"
+sh -c 'sleep 1; false' &
+pid2=$!
+wait -n
+rc=$?
+if [ "$rc" != 1 ]; then
+ echo "Test2 exit code was not 1: $rc"
+fi
+
+wait $pid2
+rc=$?
+if [ "$rc" != 1 ]; then
+ echo "Test2 exit code was not 1 on recheck: $rc"
+fi
+
+# pipeline... not too fast
+echo "Test3 (pipeline wait for whole job)"
+d1=$(date +%s)
+sleep 2 | sleep 1 &
+wait -n
+rc=$?
+if [ "$rc" != 0 ]; then
+ echo "Test3 exit code was not 0: $rc"
+fi
+d2=$(date +%s)
+if [ "$d2" -lt "$((d1 + 2))" ]; then
+ echo "Test3 finished in less than 2s"
+fi
+
+kill $bg
+echo Ok:$?
diff --git a/shell/ash_test/ash-misc/wait9.right
b/shell/ash_test/ash-misc/wait9.right
new file mode 100644
index 000000000000..23db7738d02c
--- /dev/null
+++ b/shell/ash_test/ash-misc/wait9.right
@@ -0,0 +1,6 @@
+wait -n
+from USR1
+prints after USR1
+wait
+from USR1
+prints after USR1 (2)
diff --git a/shell/ash_test/ash-misc/wait9.tests
b/shell/ash_test/ash-misc/wait9.tests
new file mode 100644
index 000000000000..3dda2e133831
--- /dev/null
+++ b/shell/ash_test/ash-misc/wait9.tests
@@ -0,0 +1,21 @@
+# tests wait and signals
+
+trap 'echo from USR1' USR1
+
+echo "wait -n"
+{ sleep 1; kill -USR1 $$; sleep 1; echo prints after USR1; } &
+wait -n
+rc=$?
+[ "$rc" = 138 ] || echo "rc not 138: $?"
+wait -n
+rc=$?
+[ "$rc" = 0 ] || echo "rc not 0: $?"
+
+echo "wait"
+{ sleep 1; kill -USR1 $$; sleep 1; echo "prints after USR1 (2)"; } &
+wait
+rc=$?
+[ "$rc" = 138 ] || echo "rc not 138: $?"
+wait
+rc=$?
+[ "$rc" = 0 ] || echo "rc not 0: $?"
--
2.47.3
_______________________________________________
busybox mailing list
[email protected]
https://lists.busybox.net/mailman/listinfo/busybox
