Re: [PATCH] eval: make traps work when "set -e" is enabled

2018-12-13 Thread Herbert Xu
Antonio Ospite  wrote:
> When "set -e" is enabled traps are not always executed, in particular
> the EXIT trap is not executed when the shell exits on an unhandled
> error.
> 
> Consider the following test script:
> 
>  #!/bin/dash
> 
>  set -e
> 
>  trap 'ret=$?; echo "EXIT: $ret"' EXIT
>  trap 'exit 2' HUP INT QUIT PIPE TERM
> 
>  read variable
> 
> By pressing Ctrl-C one would expect the EXIT trap to be called, as it is
> the case with other shells (bash, zsh), but dash does not do it.
> 
> By calling dotrap() before jumping to the exit path when checkexit is
> not zero, dash behaves like other shells.
> 
> Signed-off-by: Antonio Ospite 
> ---
> 
> Hi,
> 
> this has been reported in Debian[1] and I noticed the issue myself too, so
> I tried to take a look at it.
> 
> I am marking the patch as RFC because I don't know the dash codebase very
> well, and I might not be aware of possible drawbacks of this change. It worked
> in my limited testing but that's it.
> 
> I don't know if the behavior of traps is specified when "set -e" is active,
> but in case it isn't it would stll be good to behave like other shells.
> 
> Any comment is welcome.
> 
> Thank you,
>   Antonio
> 
> 
> [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=779416
> 
> 
> src/eval.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt


Re: [PATCH] eval: make traps work when "set -e" is enabled

2018-10-16 Thread Antonio Ospite
On Tue, 16 Oct 2018 14:09:52 +0200
Antonio Ospite  wrote:

[...]
> I am marking the patch as RFC because I don't know the dash codebase very
> well, and I might not be aware of possible drawbacks of this change. It worked
> in my limited testing but that's it.
> 

I forgot the '--rfc' option when calling git-format-patch, the subject
meant to say "[RFC PATCH] ".

Ciao,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?


[PATCH] eval: make traps work when "set -e" is enabled

2018-10-16 Thread Antonio Ospite
When "set -e" is enabled traps are not always executed, in particular
the EXIT trap is not executed when the shell exits on an unhandled
error.

Consider the following test script:

  #!/bin/dash

  set -e

  trap 'ret=$?; echo "EXIT: $ret"' EXIT
  trap 'exit 2' HUP INT QUIT PIPE TERM

  read variable

By pressing Ctrl-C one would expect the EXIT trap to be called, as it is
the case with other shells (bash, zsh), but dash does not do it.

By calling dotrap() before jumping to the exit path when checkexit is
not zero, dash behaves like other shells.

Signed-off-by: Antonio Ospite 
---

Hi,

this has been reported in Debian[1] and I noticed the issue myself too, so
I tried to take a look at it.

I am marking the patch as RFC because I don't know the dash codebase very
well, and I might not be aware of possible drawbacks of this change. It worked
in my limited testing but that's it.

I don't know if the behavior of traps is specified when "set -e" is active,
but in case it isn't it would stll be good to behave like other shells.

Any comment is welcome.

Thank you,
   Antonio


[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=779416


 src/eval.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 6185db4..7d348d0 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -307,11 +307,11 @@ setstatus:
break;
}
 out:
+   dotrap();
+
if (checkexit & status)
goto exexit;
 
-   dotrap();
-
if (flags & EV_EXIT) {
 exexit:
exraise(EXEXIT);
-- 
2.19.1