Re: [PATCH 2/2] shell: fix for 'wait -n' return 127 when nothing to wait

> > If the tests I submitted pass then I don't care whose patch are used.

Same for me. We just need the code fixed. However, my patch is a more
incremental fix while Dominique's patch refactors the logic, even
reducing the code size.

>
> +:git-shell:ec7ca9c:busybox>
> ./busybox ash shell/ash_test/ash-misc/wait_n1.tests
>
> p1 active: 1 (expects 0)
>
> all passed except the above one.
>
> https://github.com/robang74/busybox/tree/bugfixes

This is the test that failed.

# Wait for any of p1, p2. p2 finishes first.
(sleep 2; exit 41) & p1=$!
(sleep 1; exit 42) & p2=$!
wait -n $p1 $p2
echo "Wait 1 returned: $?. (expects 42)"
kill -0 $p1 2>/dev/null; echo "p1 active: $? (expects 0)"
kill -0 $p2 2>/dev/null; echo "p2 active: $? (expects 1)"

I guess it waited for p1 and not p2.

My code also handles cases like wait -n %1 %2 or wait -n $p1 $p2 using
a marker (jp->wait_n). I will also add a more comprehensive test like:

# Test wait -n with multiple PIDs
# It should return when ANY of the specified jobs finishes.

(sleep 2; exit 41) & p1=$!
(sleep 1; exit 42) & p2=$!
(sleep 3; exit 43) & p3=$!

# Should wait for p2 (1s) and return 42
wait -n $p1 $p2
echo "First wait -n returned: $? (expects 42)"

# p1 should still be running. wait -n for p1 or p3 should return when
p1 finishes (after another 1s)
wait -n $p1 $p3
echo "Second wait -n returned: $? (expects 41)"

# wait -n for p3 (still running)
wait -n $p3
echo "Third wait -n returned: $? (expects 43)"

I also tested the wait8 and wait9 tests from Dominique and got a bug
in my code. It was not a big code change but it is important.

@@ -4927,10 +4927,7 @@ waitcmd(int argc UNUSED_PARAM, char **argv)
                                * should wait for 2 seconds. Not 1 or 3.
                                */
                               if (status != -1 && !WIFSTOPPED(status)) {
-                                       retval = WEXITSTATUS(status);
-                                       if (WIFSIGNALED(status))
-                                               retval = 128 | WTERMSIG(status);
-                                       goto ret;
+                                       continue;
                               }
                       }

I will also incorporate the wait8/wait9 from Dominique test as it
covers more cases than my tests. I would also rename my wait_n? tests
to wait9, wait10... to get them inline with the other tests, although
it might get executed out of (numeric) order. I'll send a v2 with the
fix and all tests.

Now all tests pass.

ash-misc/until1.tests: ok
ash-misc/wait10.tests: ok
ash-misc/wait11.tests: ok
ash-misc/wait12.tests: ok
ash-misc/wait4.tests: ok
ash-misc/wait5.tests: ok
ash-misc/wait6.tests: ok
ash-misc/wait7.tests: ok
ash-misc/wait8.tests: ok
ash-misc/wait9.tests: ok
ash-misc/while1.tests: ok

Regards,

Luiz