(re-adding the list in cc -- there's no harm in having this public if
others follow)
Wolfgang Zekoll wrote on Wed, Nov 12, 2025 at 04:58:40AM +0000:
> I need to compare but I think my original suggestion introduces less
> change but that's only on aspect.
Right, I couldn't figue out how to keep the code minimal without growing
the size.
At this point I don't mind either way, and honestly don't know which of
"staying close to dash code" or "optimize for size" Denys prefers, so
I'll let him reply.
> The thing is this code in waitcmd() from original 1.37:
>
> ```
>
> /* man bash:
> * "When bash is waiting for an asynchronous command via
> * the wait builtin, the reception of a signal for which a trap
> * has been set will cause the wait builtin to return immediately
> * with an exit status greater than 128, immediately after which
> * the trap is executed."BASH_WAIT_N
> */
> #if BASH_WAIT_N
> status = dowait(DOWAIT_CHILD_OR_SIG | DOWAIT_JOBSTATUS, NULL);
> #else
> dowait(DOWAIT_CHILD_OR_SIG, NULL);
> #endif
> ```
>
> This looks wrong because the `status = ...` call is chosen if
> `BASH_WAIT_N` is defined, regardless if `-n` is present or not. But
> if `-n` is absent `wait` should do the normal `dowait` stuff. (Or am
> I completely wrong here?)
Ah, I broke that...
sleep 100 &
wait
no longer responds to something as basic as ctrl-C ...
Well, I guess I've earned the right to send a v3 -- just checking for
pending_sig in dowait_status() is enough:
```
diff --git a/shell/ash.c b/shell/ash.c
index 877e09bf88d7..8abf3ca72cd4 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -4573,6 +4573,8 @@ static struct job *dowait_status(void)
rjp = jp;
block = DOWAIT_NONBLOCK;
}
+ if (pending_sig)
+ break;
} while (jp || !rjp);
return rjp;
@@ -4814,7 +4816,7 @@ waitcmd(int argc UNUSED_PARAM, char **argv)
if (pending_sig)
goto sigout;
#if BASH_WAIT_N
- if (one) {
+ if (one && jp) {
/* 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.
```
(the later part is not strictly needed, as pending_sig must be set if jp
wasn't, but I think it's better safe than segfault here)
> This looks wrong because the `status = ...` call is chosen if `BASH_WAIT_N`
> is defined, regardless if `-n` is present or not. But if `-n` is absent
> `wait` should do the normal `dowait` stuff. (Or am I completely wrong here?)
>
> What I'm thinking of is to rewrite that code to something like
>
> ```
> int flags = DOWAIT_CHILD_OR_SIG;
> #if BASH_WAIT_N
> if (one)
> flags = flags | DOWAIT_JOBSTATUS;
> #endif
> status = dowait(flags, NULL);
> ```
I don't see what's wrong with the current code, or my patch if it checks
for pending_sig -- waitcmd properly prioritizes pending_sig over a
returned status, and we don't set jp->waited unless there was no signal
so there's not even a race here.
> One thing I noticed in your patch, Dominique, is that you define some
> functions in one line like
>
> static void dowait(int block, struct job *jp)
>
> instead of the usual
>
> static void
> dowait(int block, struct job *jp)
I just copied the neighboring function's style here and don't know
busybox preference.
docs/style-guide.txt doesn't seem to differentiate and a quick look
around the codebase turns up both versions, so I think Denys doesn't
particularly care either way?
> Denys, is this a kind of change you would add by yourself when
> importing patches? (Sorry if that question sound stupid but I'm
> completely new to busybox ... and git :-)).
Replying in general here but both are valid, the maintainer can do small
fixes like this to save time with a round-trip, or he could ask me to
send a new version, there's no rule.
> Then I have a final comment. During the night I noticed a missing
> test for `wait -n`. (Great, I just see, that I deleted that.) It's
> something like that
>
> ```
> echo "Test 9 (getting exit status from terminated process)"
>
> { sleep 1; exit 7; echo error; } &
> sleep 2
> wait -n
> st=$?
> [ "st" = "7" ] && echo "ok" || echo "bug"
> ```
This is already tested with Test 2 -- running `sleep 1; false` and
wait -n return code is 1, which is the exit status here.
We could change the false with an 'exit 7' if you think that's more
clear but semantically I think 0 or non-zero if fine.
What is missing though is something with a signal, so something like
trap 'echo from USR1' USR1
{ sleep 1; kill -USR1 $$; sleep 1; echo prints after USR1; } &
wait -n
echo $? # should be 138
wait -n
echo $? # should be 0
(which my v2 sent earlier looks like it works except it should prints
the message "prints after USR1" before "from USR1")
I'll send a v3 a bit later,
thanks for reporting that.
--
Dominique Martinet | Asmadeus
_______________________________________________
busybox mailing list
[email protected]
https://lists.busybox.net/mailman/listinfo/busybox
