| Message ID | 20181129113353.kx5ccakbvwnk6ptr@gondor.apana.org.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | eval: Only restore exit status on exit/return | expand |
Op 29-11-18 om 12:33 schreef Herbert Xu:
> Thanks for the report. This patch should fix the problem:
Doesn't work for me, in fact it seems to make no difference.
Here are a few more test cases.
(trap '(! :) && echo BUG1' EXIT)
(trap '(false) && echo BUG2' EXIT)
(trap 'readonly foo=bar; (foo=baz) && echo BUG3' EXIT)
(trap '(set -o bad@option) && echo BUG4' EXIT)
- M.
On Thu, Nov 29, 2018 at 03:27:31PM +0100, Martijn Dekker wrote: > Op 29-11-18 om 12:33 schreef Herbert Xu: > > Thanks for the report. This patch should fix the problem: > > Doesn't work for me, in fact it seems to make no difference. > > Here are a few more test cases. > > (trap '(! :) && echo BUG1' EXIT) > (trap '(false) && echo BUG2' EXIT) > (trap 'readonly foo=bar; (foo=baz) && echo BUG3' EXIT) > (trap '(set -o bad@option) && echo BUG4' EXIT) Oops, I only tested the dotrap case, and not exitshell. The problem is that in evalsubshell we end up in exitshell again which restores the old exit status. So we need to come up with a way to differentiate the exitshell from the original shell vs. a subshell. Thanks,
Op 29-11-18 om 15:48 schreef Herbert Xu: > The problem is that in evalsubshell we end up in exitshell again > which restores the old exit status. So we need to come up with > a way to differentiate the exitshell from the original shell vs. > a subshell. Isn't it much simpler than that? Upon forking a subshell, traps are reset, so it would make sense that any flag that says "we are currently in the process of executing a trap" is also reset. Which, from a look at the source code, seems to be the -1 value of savestatus. So this oneliner fixes it for me: diff --git a/src/trap.c b/src/trap.c index ab0ecd4..7740955 100644 --- a/src/trap.c +++ b/src/trap.c @@ -168,6 +168,7 @@ clear_traps(void) } } trapcnt = 0; + savestatus = -1; INTON; } Thanks, - M.
On Thu, Nov 29, 2018 at 04:02:32PM +0100, Martijn Dekker wrote: > > Isn't it much simpler than that? Upon forking a subshell, traps are reset, > so it would make sense that any flag that says "we are currently in the > process of executing a trap" is also reset. > > Which, from a look at the source code, seems to be the -1 value of > savestatus. So this oneliner fixes it for me: > > diff --git a/src/trap.c b/src/trap.c > index ab0ecd4..7740955 100644 > --- a/src/trap.c > +++ b/src/trap.c > @@ -168,6 +168,7 @@ clear_traps(void) > } > } > trapcnt = 0; > + savestatus = -1; > INTON; > } Hmm, I think this breaks the following case which used to work: dash -c 'trap "(:; exit) && echo BUG" EXIT; false' I know this makes no sense but almost every other shell does it this way. Thanks,
On 01/12/2018 08:45, Herbert Xu wrote: > Hmm, I think this breaks the following case which used to work: > > dash -c 'trap "(:; exit) && echo BUG" EXIT; false' > > I know this makes no sense but almost every other shell does it > this way. "Almost every other shell"? In the shells I tested, other than dash, it's evenly divided: four print nothing (bash, ksh, mksh, yash), four print BUG (bosh, pdksh, posh, zsh). Which ones are you testing? To me, the standard is unclear on what the output should be: if at the time exit is executed, traps are reset, can what's being executed still be considered a trap action? It's a nice test case whatever the correct result.
Op 01-12-18 om 11:39 schreef Harald van Dijk: > On 01/12/2018 08:45, Herbert Xu wrote: >> Hmm, I think this breaks the following case which used to work: >> >> dash -c 'trap "(:; exit) && echo BUG" EXIT; false' >> >> I know this makes no sense but almost every other shell does it >> this way. > > "Almost every other shell"? In the shells I tested, other than dash, > it's evenly divided: four print nothing (bash, ksh, mksh, yash), four > print BUG (bosh, pdksh, posh, zsh). Which ones are you testing? > > To me, the standard is unclear on what the output should be: if at the > time exit is executed, traps are reset, can what's being executed still > be considered a trap action? The word 'reset' is not ambiguous. Resetting traps in a subshell means that subshells forget all about the parent shell's traps, so the fact that the parent shell that forked that subshell happens to be executing a trap action is irrelevant to the behaviour of the subshell. Thus, the 'BUG' output is actually the correct one here. As Herbert acknowledged, the alternative makes no sense. I think the standard *is* clear. But even (or especially) if the standard is unclear, blatantly illogical behaviour should be considered a bug. > It's a nice test case whatever the correct result. Yes. - M.
Harald van Dijk <harald@gigawatt.nl> wrote: > > "Almost every other shell"? In the shells I tested, other than dash, > it's evenly divided: four print nothing (bash, ksh, mksh, yash), four > print BUG (bosh, pdksh, posh, zsh). Which ones are you testing? > > To me, the standard is unclear on what the output should be: if at the > time exit is executed, traps are reset, can what's being executed still > be considered a trap action? > > It's a nice test case whatever the correct result. I don't disagree with your reasoning. However, I'm not going to change existing behaviour unless it is clearly wrong. Cheers,
On 29/11/2018 15:02, Martijn Dekker wrote: > Op 29-11-18 om 15:48 schreef Herbert Xu: >> The problem is that in evalsubshell we end up in exitshell again >> which restores the old exit status. So we need to come up with >> a way to differentiate the exitshell from the original shell vs. >> a subshell. > > Isn't it much simpler than that? Upon forking a subshell, traps are > reset, so it would make sense that any flag that says "we are currently > in the process of executing a trap" is also reset. There's more than just trap status that should be reset in subshells. for x in a b do ( for y in c d do break 2 done echo $x ) done I expect this to print "a" and "b", possibly accompanied by some error messages from break. Instead, dash (like ksh, bosh and zsh) remembers there's an outer loop and tries to break out of that from within the subshell, which of course doesn't work. It doesn't print anything. It looks like the child process should call exitreset(), which fixes this and will also set savestatus to -1 -- except for exitreset()'s (correct) behaviour of also closing redirections.
diff --git a/src/error.h b/src/error.h index 9630b56..94e30a2 100644 --- a/src/error.h +++ b/src/error.h @@ -66,7 +66,8 @@ extern int exception; /* exceptions */ #define EXINT 0 /* SIGINT received */ #define EXERROR 1 /* a generic error */ -#define EXEXIT 4 /* exit the shell */ +#define EXEND 3 /* exit the shell */ +#define EXEXIT 4 /* exit the shell via exitcmd */ /* diff --git a/src/eval.c b/src/eval.c index 546ee1b..3cea2a3 100644 --- a/src/eval.c +++ b/src/eval.c @@ -114,12 +114,13 @@ STATIC const struct builtincmd bltin = { INCLUDE "eval.h" EXITRESET { - evalskip = 0; - loopnest = 0; if (savestatus >= 0) { - exitstatus = savestatus; + if (exception == EXEXIT || evalskip == SKIPFUNCDEF) + exitstatus = savestatus; savestatus = -1; } + evalskip = 0; + loopnest = 0; } #endif @@ -314,7 +315,7 @@ out: if (flags & EV_EXIT) { exexit: - exraise(EXEXIT); + exraise(EXEND); } return exitstatus; diff --git a/src/exec.c b/src/exec.c index 9d0215a..87354d4 100644 --- a/src/exec.c +++ b/src/exec.c @@ -143,7 +143,7 @@ shellexec(char **argv, const char *path, int idx) exitstatus = exerrno; TRACE(("shellexec failed for %s, errno %d, suppressint %d\n", argv[0], e, suppressint )); - exerror(EXEXIT, "%s: %s", argv[0], errmsg(e, E_EXEC)); + exerror(EXEND, "%s: %s", argv[0], errmsg(e, E_EXEC)); /* NOTREACHED */ } diff --git a/src/main.c b/src/main.c index 6d53e00..6b3a090 100644 --- a/src/main.c +++ b/src/main.c @@ -111,7 +111,7 @@ main(int argc, char **argv) e = exception; s = state; - if (e == EXEXIT || s == 0 || iflag == 0 || shlvl) + if (e == EXEND || e == EXEXIT || s == 0 || iflag == 0 || shlvl) exitshell(); reset();