dash.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* shell script error management in busybox ash
@ 2021-09-15 17:47 Roberto A. Foglietta
  2021-09-15 19:47 ` Denys Vlasenko
  0 siblings, 1 reply; 6+ messages in thread
From: Roberto A. Foglietta @ 2021-09-15 17:47 UTC (permalink / raw)
  To: dash; +Cc: Denys Vlasenko

[-- Attachment #1: Type: text/plain, Size: 4031 bytes --]

Dear all,

 Denys Vlasenko, the maintainer of BusyBox, asked me to organise a
task to synchronise busybox ash and dash in such a way the future
developments could benefit from each other's contributions. In
particular this need raised after these patches has been applied:

ash: add bash-like ERR trap and set -E
https://git.busybox.net/busybox/commit/?id=e0bf3df0205d5ccef52df67b1760b8b54f15ec6e

ash: fix LINENO in functions
https://git.busybox.net/busybox/commit/?id=d6c9cbc0727cc3875672ae280ec129514a9d8594

ash: LINENO starts from 0 in -c SCRIPT mode
https://git.busybox.net/busybox/commit/?id=64aa86b720641cb50be9636e6c20d4dbbea6fed0

ash: introduce bash-like $FUNCNAME
https://git.busybox.net/busybox/commit/?id=704c596563a5b4da4349b272d0c9c71aacea34a7

 These patches derive from a larger patch that I developed to improve
the error management/debug in busybox ash. In brief these patches add
the following functionalities:

 - trap ERR and set -E added
 - LINENO became a global variable
 - FUNCNAME global variable added

 Developing these patches we realised that the use of EXEXIT and EXEND
exceptions complicates the code and do not add any value. Moreover,
the code that these exceptions trigger (everywhere the setjmp()
returns true) restores some values and frees some memory but the traps
need the information before the restore, not the restored values. For
these reason I developed three patches here in attachment:

 1. get rid of EXEXIT in exitcmd() and fix a bug about return value
 2. get rid of EXEND at the end of evaltree()
 3. remove the EXEXIT and EXEND altogether

 The description of these patches are simply arbitrary and temporary.
I hope we will agree on these details once we are going to port them
to dash.

 Applying these patches the output printed by trap ERR and trap EXIT
is what was expected to be demonstrating that to avoid these
exceptions the code became straightforward. We are here to check your
interest in porting these patches to dash and your will to apply.

 Our focus is related to the patches listed above but in the long run
it is not limited to them but also includes the following patches.
These patches have been applied in the meantime that others made
changes to ash.c as well. The full list of patches and their order is
listed here:

https://git.busybox.net/busybox/log/

 Other patches that have been applied that might be a convenience to
port in dash are the following:

ash: eval: Check nflag in evaltree instead of cmdloop
https://git.busybox.net/busybox/commit/?id=41beb53787ec798a27f336c4758cb5ebd8f0c75a

ash: eval: Do not cache value of eflag in evaltree
https://git.busybox.net/busybox/commit/?id=f415e21a7dce1d4f4b760fddfaba85c551681e11

ash: parser: Fix handling of empty aliases
https://git.busybox.net/busybox/commit/?id=30af5938afad076e12b8ece123cab0b8bc92a596

ash: parser: Save and restore heredoclist in expandstr
https://git.busybox.net/busybox/commit/?id=1c06ddd8bbbd6906e5bf00ec93e04d5090718be9

ash: use pgetc_eatbnl() in more places, take 3
https://git.busybox.net/busybox/commit/?id=c54025612711a6b1997efd99206b9fbcaa5a29cf

ash: parser: Fix alias expansion after heredoc or newlines
https://git.busybox.net/busybox/commit/?id=8c68ae8416c8b54222eb3cd1d4908a570147e134

ash: parser: Get rid of PEOA
https://git.busybox.net/busybox/commit/?id=48cb983b136fb74c61db594a30e18bdc42b7264c

ash: eval: Prevent recursive PS4 expansion
https://git.busybox.net/busybox/commit/?id=eb607777697f4c5eb2dfd86e5837a8c379f65979

ash: fix ignoreeof option
https://git.busybox.net/busybox/commit/?id=0beee209778870888c3a80a9ae57e74888bc8e7b

ash: stopped jobs should only prevent exit from interactive shell
https://git.busybox.net/busybox/commit/?id=50239a665c88f5a95ce41146804500f5da90b19e

ash: let ignoreeof only affect interactive shells
https://git.busybox.net/busybox/commit/?id=5726df5f94f973eaa097d9853ceff2bd6b748d97

 I wish to receive your feedback and I hope you are interested in the porting.

 Best regards,
--
Roberto A. Foglietta
+39.349.33.30.697

[-- Attachment #2: 0003-EXEND-and-EXEXIT-removed.patch --]
[-- Type: application/x-patch, Size: 1978 bytes --]

[-- Attachment #3: 0001-fix-a-bug-in-exitcmd-raised-by-Harald-with-s-EXEXIT-.patch --]
[-- Type: application/x-patch, Size: 1059 bytes --]

[-- Attachment #4: 0002-FUNCNAME-bugfix-with-s-EXEND-exitshell.patch --]
[-- Type: application/x-patch, Size: 592 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: shell script error management in busybox ash
  2021-09-15 17:47 shell script error management in busybox ash Roberto A. Foglietta
@ 2021-09-15 19:47 ` Denys Vlasenko
  2021-09-15 19:59   ` Harald van Dijk
  0 siblings, 1 reply; 6+ messages in thread
From: Denys Vlasenko @ 2021-09-15 19:47 UTC (permalink / raw)
  To: Roberto A. Foglietta; +Cc: DASH shell mailing list

On Wed, Sep 15, 2021 at 7:48 PM Roberto A. Foglietta
<roberto.foglietta@gmail.com> wrote:
> Dear all,
>
>  Denys Vlasenko, the maintainer of BusyBox, asked me to organise a
> task to synchronise busybox ash and dash in such a way the future
> developments could benefit from each other's contributions. In
> particular this need raised after these patches has been applied:
...
>  Developing these patches we realised that the use of EXEXIT and EXEND
> exceptions complicates the code and do not add any value.

Actually there was no discussion with me to agree on such an opinion.
There may be value in it.

I didn't look too closely, but backpropagating of fatal exceptions
might be necessary to restore foreground process group on exit?
(grep for tcsetpgrp). Maybe something else too. Need to take
a very good look before deciding on "it's useless".


> Moreover,
> the code that these exceptions trigger (everywhere the setjmp()
> returns true) restores some values and frees some memory but the traps
> need the information before the restore, not the restored values.

Yes, _this_ is the problem we are having.

E.g. $FUNCNAME in EXIT trap needs to be current function's name.
With how EXEXIT exception is propagating back in current dash,
it's difficult to arrange for that: the trap is run well after
we leave the "execute this function" dash code.

The solution may be to change exit builtin handling from
"throw exraise(EXEXIT) exception, it will run EXIT trap (among other things)"
to
"run EXIT trap, then throw exraise(EXEXIT) exception
(which _will not_ run EXIT trap)"

> For
> these reason I developed three patches here in attachment:
>
>  1. get rid of EXEXIT in exitcmd() and fix a bug about return value
>  2. get rid of EXEND at the end of evaltree()
>  3. remove the EXEXIT and EXEND altogether

I would imagine if you propose changes to dash,
the patches should be against dash?


>  The description of these patches are simply arbitrary and temporary.

In other words, "my patches are not ready"... why posting them then?


> I hope we will agree on these details once we are going to port them
> to dash.
>
>  Applying these patches the output printed by trap ERR and trap EXIT
> is what was expected to be demonstrating that to avoid these
> exceptions the code became straightforward. We are here to check your
> interest in porting these patches to dash and your will to apply.
>
>  Our focus is related to the patches listed above but in the long run
> it is not limited to them but also includes the following patches.
> These patches have been applied in the meantime that others made
> changes to ash.c as well. The full list of patches and their order is
> listed here:
>
> https://git.busybox.net/busybox/log/
>
>  Other patches that have been applied that might be a convenience to
> port in dash are the following:
>
> ash: eval: Check nflag in evaltree instead of cmdloop
> https://git.busybox.net/busybox/commit/?id=41beb53787ec798a27f336c4758cb5ebd8f0c75a
>
> ash: eval: Do not cache value of eflag in evaltree
> https://git.busybox.net/busybox/commit/?id=f415e21a7dce1d4f4b760fddfaba85c551681e11
>
> ash: parser: Fix handling of empty aliases
> https://git.busybox.net/busybox/commit/?id=30af5938afad076e12b8ece123cab0b8bc92a596
>
> ash: parser: Save and restore heredoclist in expandstr
> https://git.busybox.net/busybox/commit/?id=1c06ddd8bbbd6906e5bf00ec93e04d5090718be9
>
> ash: use pgetc_eatbnl() in more places, take 3
> https://git.busybox.net/busybox/commit/?id=c54025612711a6b1997efd99206b9fbcaa5a29cf
>
> ash: parser: Fix alias expansion after heredoc or newlines
> https://git.busybox.net/busybox/commit/?id=8c68ae8416c8b54222eb3cd1d4908a570147e134
>
> ash: parser: Get rid of PEOA
> https://git.busybox.net/busybox/commit/?id=48cb983b136fb74c61db594a30e18bdc42b7264c
>
> ash: eval: Prevent recursive PS4 expansion
> https://git.busybox.net/busybox/commit/?id=eb607777697f4c5eb2dfd86e5837a8c379f65979

None of the above needs porting to dash as it already came from dash.


> ash: fix ignoreeof option
> https://git.busybox.net/busybox/commit/?id=0beee209778870888c3a80a9ae57e74888bc8e7b
>
> ash: stopped jobs should only prevent exit from interactive shell
> https://git.busybox.net/busybox/commit/?id=50239a665c88f5a95ce41146804500f5da90b19e
>
> ash: let ignoreeof only affect interactive shells
> https://git.busybox.net/busybox/commit/?id=5726df5f94f973eaa097d9853ceff2bd6b748d97

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: shell script error management in busybox ash
  2021-09-15 19:47 ` Denys Vlasenko
@ 2021-09-15 19:59   ` Harald van Dijk
  2021-09-15 20:07     ` Denys Vlasenko
  0 siblings, 1 reply; 6+ messages in thread
From: Harald van Dijk @ 2021-09-15 19:59 UTC (permalink / raw)
  To: Denys Vlasenko, Roberto A. Foglietta; +Cc: DASH shell mailing list

On 15/09/2021 20:47, Denys Vlasenko wrote:
> On Wed, Sep 15, 2021 at 7:48 PM Roberto A. Foglietta
> <roberto.foglietta@gmail.com> wrote:
>> Moreover,
>> the code that these exceptions trigger (everywhere the setjmp()
>> returns true) restores some values and frees some memory but the traps
>> need the information before the restore, not the restored values.
> 
> Yes, _this_ is the problem we are having.
> 
> E.g. $FUNCNAME in EXIT trap needs to be current function's name.

Important to also mention that although FUNCNAME is not part of POSIX, 
and there is no spec to compare to, in bash and ksh it does not behave 
that way, so you're talking about implementing FUNCNAME in a way that is 
incompatible with existing shells. Which is fine if that is what you 
want to do.

Cheers,
Harald van Dijk

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: shell script error management in busybox ash
  2021-09-15 19:59   ` Harald van Dijk
@ 2021-09-15 20:07     ` Denys Vlasenko
  2021-09-15 20:16       ` Harald van Dijk
  0 siblings, 1 reply; 6+ messages in thread
From: Denys Vlasenko @ 2021-09-15 20:07 UTC (permalink / raw)
  To: Harald van Dijk; +Cc: Roberto A. Foglietta, DASH shell mailing list

On Wed, Sep 15, 2021 at 9:59 PM Harald van Dijk <harald@gigawatt.nl> wrote:
> On 15/09/2021 20:47, Denys Vlasenko wrote:
> > On Wed, Sep 15, 2021 at 7:48 PM Roberto A. Foglietta
> > <roberto.foglietta@gmail.com> wrote:
> >> Moreover,
> >> the code that these exceptions trigger (everywhere the setjmp()
> >> returns true) restores some values and frees some memory but the traps
> >> need the information before the restore, not the restored values.
> >
> > Yes, _this_ is the problem we are having.
> >
> > E.g. $FUNCNAME in EXIT trap needs to be current function's name.
>
> Important to also mention that although FUNCNAME is not part of POSIX,
> and there is no spec to compare to, in bash and ksh it does not behave
> that way

In my testing, it does.

> so you're talking about implementing FUNCNAME in a way that is
> incompatible with existing shells.

Try this in bash:

  trap 'echo trap:$FUNCNAME' EXIT
  f() { exit; }
  f

I'm getting:
  trap:f

IOW: in EXIT trap, $FUNCNAME is the name of the function where "exit"
was invoked.

$ bash --version
GNU bash, version 5.0.17(1)-release (x86_64-redhat-linux-gnu)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: shell script error management in busybox ash
  2021-09-15 20:07     ` Denys Vlasenko
@ 2021-09-15 20:16       ` Harald van Dijk
  2021-09-15 20:22         ` Denys Vlasenko
  0 siblings, 1 reply; 6+ messages in thread
From: Harald van Dijk @ 2021-09-15 20:16 UTC (permalink / raw)
  To: Denys Vlasenko; +Cc: Roberto A. Foglietta, DASH shell mailing list

On 15/09/2021 21:07, Denys Vlasenko wrote:
> On Wed, Sep 15, 2021 at 9:59 PM Harald van Dijk <harald@gigawatt.nl> wrote:
>> On 15/09/2021 20:47, Denys Vlasenko wrote:
>>> On Wed, Sep 15, 2021 at 7:48 PM Roberto A. Foglietta
>>> <roberto.foglietta@gmail.com> wrote:
>>>> Moreover,
>>>> the code that these exceptions trigger (everywhere the setjmp()
>>>> returns true) restores some values and frees some memory but the traps
>>>> need the information before the restore, not the restored values.
>>>
>>> Yes, _this_ is the problem we are having.
>>>
>>> E.g. $FUNCNAME in EXIT trap needs to be current function's name.
>>
>> Important to also mention that although FUNCNAME is not part of POSIX,
>> and there is no spec to compare to, in bash and ksh it does not behave
>> that way
> 
> In my testing, it does.
> 
>> so you're talking about implementing FUNCNAME in a way that is
>> incompatible with existing shells.
> 
> Try this in bash:
> 
>    trap 'echo trap:$FUNCNAME' EXIT
>    f() { exit; }
>    f
> 
> I'm getting:
>    trap:f

For bash, it depends on how the shell is invoked. When bash reads 
commands from stdin, it prints trap:f. When bash is called with that 
exact same script passed as -c, it prints trap:. There may be a more 
specific option that can control this.

$ bash --version
GNU bash, version 5.1.4(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
<http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

For ksh, as far as I can tell it always prints trap:.

$ ksh --version
   version         sh (AT&T Research) 93u+ 2012-08-01

Cheers,
Harald van Dijk

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: shell script error management in busybox ash
  2021-09-15 20:16       ` Harald van Dijk
@ 2021-09-15 20:22         ` Denys Vlasenko
  0 siblings, 0 replies; 6+ messages in thread
From: Denys Vlasenko @ 2021-09-15 20:22 UTC (permalink / raw)
  To: Harald van Dijk; +Cc: Roberto A. Foglietta, DASH shell mailing list

On Wed, Sep 15, 2021 at 10:16 PM Harald van Dijk <harald@gigawatt.nl> wrote:
> >>> Yes, _this_ is the problem we are having.
> >>>
> >>> E.g. $FUNCNAME in EXIT trap needs to be current function's name.
> >>
> >> Important to also mention that although FUNCNAME is not part of POSIX,
> >> and there is no spec to compare to, in bash and ksh it does not behave
> >> that way
> >
> > In my testing, it does.
> >
> >> so you're talking about implementing FUNCNAME in a way that is
> >> incompatible with existing shells.
> >
> > Try this in bash:
> >
> >    trap 'echo trap:$FUNCNAME' EXIT
> >    f() { exit; }
> >    f
> >
> > I'm getting:
> >    trap:f
>
> For bash, it depends on how the shell is invoked. When bash reads
> commands from stdin, it prints trap:f. When bash is called with that
> exact same script passed as -c, it prints trap:. There may be a more
> specific option that can control this.

Indeed. In -c, it prints "trap:"

In a script, it prints "trap:f"

Looks quite inconsistent.

Well, in this case I'd fall back to "what would be the most _useful_
behavior". Losing information in which function "exit" was is less useful.
I prefer passing it to EXIT trap.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-15 20:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-15 17:47 shell script error management in busybox ash Roberto A. Foglietta
2021-09-15 19:47 ` Denys Vlasenko
2021-09-15 19:59   ` Harald van Dijk
2021-09-15 20:07     ` Denys Vlasenko
2021-09-15 20:16       ` Harald van Dijk
2021-09-15 20:22         ` Denys Vlasenko

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