[Qemu-devel] [PATCH] Don't exit with zero in the trap handler

[Qemu-devel] [PATCH] Don't exit with zero in the trap handler

[Loïc Minier]

When configure runs "exit 1", the trap handler is run to cleanup any
files created by configure, but this trap handler itself calls "exit"
with no argument (which means zero exit code):
[...]
+ echo Error: zlib check failed
Error: zlib check failed
+ echo Make sure to have the zlib libs and headers installed.
Make sure to have the zlib libs and headers installed.
+ echo

+ exit 1
+ rm -f /tmp/qemu-conf--2779-.c /tmp/qemu-conf--2779-.o
/tmp/qemu-conf--2779-.exe
+ exit

To fix this, remove the call to exit from the trap handler, leaving it
to the shell shell to exitafter the trap handler is run (honoring any
previously provided exit code).
---
 configure |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index 3bfc5e9..e0147d1 100755
--- a/configure
+++ b/configure
@@ -15,7 +15,7 @@ TMPC="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.c"
 TMPO="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.o"
 TMPE="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.exe"
 
-trap "rm -f $TMPC $TMPO $TMPE ; exit" EXIT INT QUIT TERM
+trap "rm -f $TMPC $TMPO $TMPE" EXIT INT QUIT TERM
 
 compile_object() {
   $cc $QEMU_CFLAGS -c -o $TMPO $TMPC > /dev/null 2> /dev/null
-- 
1.7.1

Re: [Qemu-devel] [PATCH] Don't exit with zero in the trap handler

[Markus Armbruster]

Loïc Minier <loic.minier@linaro.org> writes:

> When configure runs "exit 1", the trap handler is run to cleanup any
> files created by configure, but this trap handler itself calls "exit"
> with no argument (which means zero exit code):
> [...]
> + echo Error: zlib check failed
> Error: zlib check failed
> + echo Make sure to have the zlib libs and headers installed.
> Make sure to have the zlib libs and headers installed.
> + echo
>
> + exit 1
> + rm -f /tmp/qemu-conf--2779-.c /tmp/qemu-conf--2779-.o
> /tmp/qemu-conf--2779-.exe
> + exit
>
> To fix this, remove the call to exit from the trap handler, leaving it
> to the shell shell to exitafter the trap handler is run (honoring any
> previously provided exit code).

This suggests the old code screws up the exit code.  It doesn't for me.
Unless it does at least on some platforms, it's a cleanup, not a fix,
and the commit message should reflect that.

Re: [Qemu-devel] [PATCH] Don't exit with zero in the trap handler

[Loïc Minier]

On Sat, Sep 25, 2010, Markus Armbruster wrote:
> This suggests the old code screws up the exit code.  It doesn't for me.
> Unless it does at least on some platforms, it's a cleanup, not a fix,
> and the commit message should reflect that.

 It does screw up the exit code for me; it seems it's because dash is
 used as /bin/sh.  If I call this shell snippet:
    trap "echo trap; exit" 0 1 2 3 9 11 13 15
    exit 2
 with dash, e.g. "dash foo.sh; echo $?", I get 0, and with bash I get 2.

 I'm not sure what POSIX says, but given that calling exit in a trap
 handler isn't needed here, I recommend including this as a bug fix.

-- 
Loïc Minier

Re: [Qemu-devel] [PATCH] Don't exit with zero in the trap handler

[Blue Swirl]

On Sat, Sep 25, 2010 at 8:31 AM, Loïc Minier <lool@dooz.org> wrote:
> On Sat, Sep 25, 2010, Markus Armbruster wrote:
>> This suggests the old code screws up the exit code.  It doesn't for me.
>> Unless it does at least on some platforms, it's a cleanup, not a fix,
>> and the commit message should reflect that.
>
>  It does screw up the exit code for me; it seems it's because dash is
>  used as /bin/sh.  If I call this shell snippet:
>    trap "echo trap; exit" 0 1 2 3 9 11 13 15
>    exit 2
>  with dash, e.g. "dash foo.sh; echo $?", I get 0, and with bash I get 2.
>
>  I'm not sure what POSIX says, but given that calling exit in a trap
>  handler isn't needed here, I recommend including this as a bug fix.

http://www.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_21

"When exit is executed in a trap action, the last command is
considered to be the command that executed immediately preceding the
trap action."

It looks like dash and ksh are not compliant and use the return value
of echo or rm inside trap handler:
dash -c 'trap "sh -c \"exit 4\"; exit" 0 1 2 3 9 11 13 15;exit 3'; echo $?
4
ksh -c 'trap "sh -c \"exit 4\"; exit" 0 1 2 3 9 11 13 15;exit 3'; echo $?
4
bash -c 'trap "sh -c \"exit 4\"; exit" 0 1 2 3 9 11 13 15;exit 3'; echo $?
3

Re: [Qemu-devel] [PATCH] Don't exit with zero in the trap handler

[Loïc Minier]

On Sat, Sep 25, 2010, Blue Swirl wrote:
> It looks like dash and ksh are not compliant and use the return value
> of echo or rm inside trap handler:
> dash -c 'trap "sh -c \"exit 4\"; exit" 0 1 2 3 9 11 13 15;exit 3'; echo $?
> 4

 I've filed https://bugs.launchpad.net/dash/+bug/647450 to track this
 and forwarded the bug to dash@vger.kernel.org.

> ksh -c 'trap "sh -c \"exit 4\"; exit" 0 1 2 3 9 11 13 15;exit 3'; echo $?
> 4

 On my system, ksh is provided by zsh and zsh gets this right.


 In the mean time, could you please pull the patch in QEMU?  Without
 "exit" in the trap handler, the correct exit code will be returned.

    Thanks,
-- 
Loïc Minier

Re: [Qemu-devel] [PATCH] Don't exit with zero in the trap handler

[Markus Armbruster]

Loïc Minier <lool@dooz.org> writes:

> On Sat, Sep 25, 2010, Blue Swirl wrote:
>> It looks like dash and ksh are not compliant and use the return value
>> of echo or rm inside trap handler:
>> dash -c 'trap "sh -c \"exit 4\"; exit" 0 1 2 3 9 11 13 15;exit 3'; echo $?
>> 4
>
>  I've filed https://bugs.launchpad.net/dash/+bug/647450 to track this
>  and forwarded the bug to dash@vger.kernel.org.
>
>> ksh -c 'trap "sh -c \"exit 4\"; exit" 0 1 2 3 9 11 13 15;exit 3'; echo $?
>> 4
>
>  On my system, ksh is provided by zsh and zsh gets this right.

So it's a cleanup that also works around a bug in dash.

>  In the mean time, could you please pull the patch in QEMU?  Without
>  "exit" in the trap handler, the correct exit code will be returned.

Would you mind resending with a more suitable commit message?

Re: [Qemu-devel] [PATCH] Don't exit with zero in the trap handler

[Blue Swirl]

On Sat, Sep 25, 2010 at 12:01 PM, Loïc Minier <lool@dooz.org> wrote:
> On Sat, Sep 25, 2010, Blue Swirl wrote:
>> It looks like dash and ksh are not compliant and use the return value
>> of echo or rm inside trap handler:
>> dash -c 'trap "sh -c \"exit 4\"; exit" 0 1 2 3 9 11 13 15;exit 3'; echo $?
>> 4
>
>  I've filed https://bugs.launchpad.net/dash/+bug/647450 to track this
>  and forwarded the bug to dash@vger.kernel.org.
>
>> ksh -c 'trap "sh -c \"exit 4\"; exit" 0 1 2 3 9 11 13 15;exit 3'; echo $?
>> 4
>
>  On my system, ksh is provided by zsh and zsh gets this right.

On OpenBSD, ksh is pdksh and it fails like dash.

>  In the mean time, could you please pull the patch in QEMU?  Without
>  "exit" in the trap handler, the correct exit code will be returned.

I tried also saving the return value, but it does not work:
dash -c 'trap "ret=$?;echo ret: $ret;sh -c \"exit 4\";exit $ret" 0 1 2
3 9 11 13 15;sh -c "exit 5"; echo exit now $?;exit'; echo $?
exit now 5
ret:
4
ksh -c 'trap "ret=$?;echo ret: $ret;sh -c \"exit 4\";exit $ret" 0 1 2
3 9 11 13 15;sh -c "exit 5"; echo exit now $?;exit'; echo $?
exit now 5
ret:
4
bash -c 'trap "ret=$?;echo ret: $ret;sh -c \"exit 4\";exit $ret" 0 1 2
3 9 11 13 15;sh -c "exit 5"; echo exit now $?;exit'; echo $?
exit now 5
ret:
0

Perhaps a short comment should be added to warn against adding exit.

[Qemu-devel] [PATCH] Avoid exit in trap as it breaks with some shells

[Loïc Minier]

Don't call exit in the trap handler as it causes the return code to be
zero with some buggy shells (dash and pdksh at least) and is useless
here anyway.
---
 configure |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/configure b/configure
index 3bfc5e9..9d3acfc 100755
--- a/configure
+++ b/configure
@@ -15,7 +15,9 @@ TMPC="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.c"
 TMPO="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.o"
 TMPE="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.exe"
 
-trap "rm -f $TMPC $TMPO $TMPE ; exit" EXIT INT QUIT TERM
+# NB: do not call "exit" in the trap handler; this is buggy with some shells;
+# see <1285349658-3122-1-git-send-email-loic.minier@linaro.org>
+trap "rm -f $TMPC $TMPO $TMPE" EXIT INT QUIT TERM
 
 compile_object() {
   $cc $QEMU_CFLAGS -c -o $TMPO $TMPC > /dev/null 2> /dev/null
-- 
1.7.1

Re: [Qemu-devel] [PATCH] Avoid exit in trap as it breaks with some shells

[Blue Swirl]

Thanks, applied. Please remember to add the Signed-off-line.

On Sat, Sep 25, 2010 at 7:52 PM, Loïc Minier <loic.minier@linaro.org> wrote:
> Don't call exit in the trap handler as it causes the return code to be
> zero with some buggy shells (dash and pdksh at least) and is useless
> here anyway.
> ---
>  configure |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/configure b/configure
> index 3bfc5e9..9d3acfc 100755
> --- a/configure
> +++ b/configure
> @@ -15,7 +15,9 @@ TMPC="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.c"
>  TMPO="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.o"
>  TMPE="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.exe"
>
> -trap "rm -f $TMPC $TMPO $TMPE ; exit" EXIT INT QUIT TERM
> +# NB: do not call "exit" in the trap handler; this is buggy with some shells;
> +# see <1285349658-3122-1-git-send-email-loic.minier@linaro.org>
> +trap "rm -f $TMPC $TMPO $TMPE" EXIT INT QUIT TERM
>
>  compile_object() {
>   $cc $QEMU_CFLAGS -c -o $TMPO $TMPC > /dev/null 2> /dev/null
> --
> 1.7.1
>
>
>

Re: [Qemu-devel] [PATCH] Avoid exit in trap as it breaks with some shells

[Peter Maydell]

On 25 September 2010 20:52, Loïc Minier <loic.minier@linaro.org> wrote:
> Don't call exit in the trap handler as it causes the return code to be
> zero with some buggy shells (dash and pdksh at least) and is useless
> here anyway.
> ---
>  configure |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/configure b/configure
> index 3bfc5e9..9d3acfc 100755
> --- a/configure
> +++ b/configure
> @@ -15,7 +15,9 @@ TMPC="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.c"
>  TMPO="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.o"
>  TMPE="${TMPDIR1}/qemu-conf-${RANDOM}-$$-${RANDOM}.exe"
>
> -trap "rm -f $TMPC $TMPO $TMPE ; exit" EXIT INT QUIT TERM
> +# NB: do not call "exit" in the trap handler; this is buggy with some shells;
> +# see <1285349658-3122-1-git-send-email-loic.minier@linaro.org>
> +trap "rm -f $TMPC $TMPO $TMPE" EXIT INT QUIT TERM
>
>  compile_object() {
>    $cc $QEMU_CFLAGS -c -o $TMPO $TMPC > /dev/null 2> /dev/null
> --
> 1.7.1

Dragging up this ancient thread from 2010 to note that this
change turns out to have an undesirable effect: if you hit
^C at QEMU, the trap handler will run (good) but the shell
won't actually exit (bad!), and we will continue to run through
the rest of the configure script after removing the temporaries.

I guess we need to figure out an alternate approach to the
problem noted in the msgid/commit message.

thanks
-- PMM