* trap overwrites last exit code
@ 2021-05-06 9:55 Patrick Brünn
2021-05-17 7:19 ` eval: Do not cache value of eflag in evaltree Herbert Xu
0 siblings, 1 reply; 3+ messages in thread
From: Patrick Brünn @ 2021-05-06 9:55 UTC (permalink / raw)
To: dash
Hello list,
Since we are migrating to Debian bullseye, we discovered a new behavior
with our scripts, which look like this:
>#!/bin/sh
>cleanup() {
> set +e
> rmdir ""
>}
>set -eu
>trap 'cleanup' EXIT INT TERM
>echo 'Hello world!'
With old dash v0.5.10.2 this script would return 0 as we expected it.
But since commit 62cf6955f8abe875752d7163f6f3adbc7e49ebae it returns
the last exit code of our cleanup function.
Reverting that commit gives a merge conflict, but it seems to fix _our_
problem. As that topic appears too complex to us I want to ask the
experts here:
Is this change in behavior intended, by dash?
Our workaround at the moment would be:
>trap 'cleanup || true' EXIT INT TERM
Comments on our usage of traps are welcome, too.
Thanks and best regards,
Patrick
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
^ permalink raw reply [flat|nested] 3+ messages in thread
* eval: Do not cache value of eflag in evaltree
2021-05-06 9:55 trap overwrites last exit code Patrick Brünn
@ 2021-05-17 7:19 ` Herbert Xu
2021-05-19 8:08 ` Patrick Brünn
0 siblings, 1 reply; 3+ messages in thread
From: Herbert Xu @ 2021-05-17 7:19 UTC (permalink / raw)
To: Patrick Brünn; +Cc: dash
Patrick Brünn <P.Bruenn@beckhoff.com> wrote:
>
> Since we are migrating to Debian bullseye, we discovered a new behavior
> with our scripts, which look like this:
>>#!/bin/sh
>>cleanup() {
>> set +e^M
>> rmdir ""
>>}
>>set -eu
>>trap 'cleanup' EXIT INT TERM
>>echo 'Hello world!'
>
> With old dash v0.5.10.2 this script would return 0 as we expected it.
> But since commit 62cf6955f8abe875752d7163f6f3adbc7e49ebae it returns
> the last exit code of our cleanup function.
> Reverting that commit gives a merge conflict, but it seems to fix _our_
> problem. As that topic appears too complex to us I want to ask the
> experts here:
>
> Is this change in behavior intended, by dash?
>
> Our workaround at the moment would be:
>>trap 'cleanup || true' EXIT INT TERM
Thanks for the report. This is actually a fairly old bug with
set -e that's just been exposed by the exit status change. What's
really happening is that cleanup itself is triggering a set -e
exit incorrectly because evaltree cached the value of eflag prior
to the function call.
This patch should fix the problem.
Reported-by: Patrick Brünn <P.Bruenn@beckhoff.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/src/eval.c b/src/eval.c
index 9476fbb..3337f71 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -252,18 +252,10 @@ evaltree(union node *n, int flags)
popredir(0);
goto setstatus;
case NCMD:
-#ifdef notyet
- if (eflag && !(flags & EV_TESTED))
- checkexit = ~0;
- status = evalcommand(n, flags, (struct backcmd *)NULL);
- goto setstatus;
-#else
evalfn = evalcommand;
checkexit:
- if (eflag && !(flags & EV_TESTED))
- checkexit = ~0;
+ checkexit = ~flags & EV_TESTED;
goto calleval;
-#endif
case NFOR:
evalfn = evalfor;
goto calleval;
@@ -323,7 +315,7 @@ setstatus:
out:
dotrap();
- if (checkexit & status)
+ if (eflag && checkexit && status)
goto exexit;
if (flags & EV_EXIT) {
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: eval: Do not cache value of eflag in evaltree
2021-05-17 7:19 ` eval: Do not cache value of eflag in evaltree Herbert Xu
@ 2021-05-19 8:08 ` Patrick Brünn
0 siblings, 0 replies; 3+ messages in thread
From: Patrick Brünn @ 2021-05-19 8:08 UTC (permalink / raw)
To: herbert; +Cc: dash
On Mon, 2021-05-17 at 15:19 +0800, Herbert Xu wrote:
> Patrick Brünn <P.Bruenn@beckhoff.com> wrote:
> >
> > Since we are migrating to Debian bullseye, we discovered a new behavior
> > with our scripts, which look like this:
> > > #!/bin/sh
> > > cleanup() {
> > > set +e^M
> > > rmdir ""
> > > }
> > > set -eu
> > > trap 'cleanup' EXIT INT TERM
> > > echo 'Hello world!'
> >
> > With old dash v0.5.10.2 this script would return 0 as we expected it.
> > But since commit 62cf6955f8abe875752d7163f6f3adbc7e49ebae it returns
> > the last exit code of our cleanup function.
> > Reverting that commit gives a merge conflict, but it seems to fix _our_
> > problem. As that topic appears too complex to us I want to ask the
> > experts here:
> >
> > Is this change in behavior intended, by dash?
> >
> > Our workaround at the moment would be:
> > > trap 'cleanup || true' EXIT INT TERM
>
> Thanks for the report. This is actually a fairly old bug with
> set -e that's just been exposed by the exit status change. What's
> really happening is that cleanup itself is triggering a set -e
> exit incorrectly because evaltree cached the value of eflag prior
> to the function call.
>
> This patch should fix the problem.
>
> Reported-by: Patrick Brünn <P.Bruenn@beckhoff.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/src/eval.c b/src/eval.c
> index 9476fbb..3337f71 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -252,18 +252,10 @@ evaltree(union node *n, int flags)
> popredir(0);
> goto setstatus;
> case NCMD:
> -#ifdef notyet
> - if (eflag && !(flags & EV_TESTED))
> - checkexit = ~0;
> - status = evalcommand(n, flags, (struct backcmd *)NULL);
> - goto setstatus;
> -#else
> evalfn = evalcommand;
> checkexit:
> - if (eflag && !(flags & EV_TESTED))
> - checkexit = ~0;
> + checkexit = ~flags & EV_TESTED;
> goto calleval;
> -#endif
> case NFOR:
> evalfn = evalfor;
> goto calleval;
> @@ -323,7 +315,7 @@ setstatus:
> out:
> dotrap();
>
> - if (checkexit & status)
> + if (eflag && checkexit && status)
> goto exexit;
>
> if (flags & EV_EXIT) {
> --
Working for us, thanks!
Tested-by: Patrick Brünn <P.Bruenn@beckhoff.com>
Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-19 8:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 9:55 trap overwrites last exit code Patrick Brünn
2021-05-17 7:19 ` eval: Do not cache value of eflag in evaltree Herbert Xu
2021-05-19 8:08 ` Patrick Brünn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).