dash.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Herbert Xu <herbert@gondor.apana.org.au>
To: "Patrick Brünn" <P.Bruenn@beckhoff.com>
Cc: dash@vger.kernel.org
Subject: eval: Do not cache value of eflag in evaltree
Date: Mon, 17 May 2021 15:19:23 +0800	[thread overview]
Message-ID: <20210517071923.vv3qfm23mven7je3@gondor.apana.org.au> (raw)
In-Reply-To: <7ee35167691c1b1e42100f91842f07cbe817b337.camel@beckhoff.com>

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

  reply	other threads:[~2021-05-17  7:19 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-06  9:55 trap overwrites last exit code Patrick Brünn
2021-05-17  7:19 ` Herbert Xu [this message]
2021-05-19  8:08   ` eval: Do not cache value of eflag in evaltree Patrick Brünn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210517071923.vv3qfm23mven7je3@gondor.apana.org.au \
    --to=herbert@gondor.apana.org.au \
    --cc=P.Bruenn@beckhoff.com \
    --cc=dash@vger.kernel.org \
    --subject='Re: eval: Do not cache value of eflag in evaltree' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).