[PATCH] shell script error management in ash (set of 6 patches)

[Harald van Dijk]

On 24/08/2021 11:10, Roberto A. Foglietta wrote:
> Thanks again for your insightfulness.
> 
> I reverted back to the previous situation when I was resetting the 
> recursive flag when the evaltree returns:
> 
>   - if evaltree returns: no problem
>   - before an not-exit not-returning exception is raised, it sets an 
> handler that makes it returning
> 
>   Moreover, I used static variables to save the previous state in such a 
> way I am able to set back again in case my handler takes off.
>   I have updated the testsuite and moved the test9.sh in testD.sh plus 
> testD.sh makes the check of the last case you arise to my attention.
> 
>   The full patch is here:
> 
> https://github.com/robang74/tinycore-editor/tree/main/busybox/patches 
> <https://github.com/robang74/tinycore-editor/tree/main/busybox/patches>
>   busybox-1.33.1-error-management-extension-for-ash-v017.patch

I had trouble applying the patch you attached, so I reset everything and 
restarted from that full patch. I'm getting:

   shell/ash.c: In function ‘evalfun’:
   shell/ash.c:9910:16: warning: variable ‘evaldone’ set but not used 
[-Wunused-but-set-variable]
    9910 |         int e, evaldone = 0;
         |                ^~~~~~~~

That looks like a correct warning, evaldone is never read, only assigned to.

As for the handling of errors, you are now catching EXERROR and not 
propagating the error. That does not look right. However, when trying to 
create a testcase for that, I stumbled upon a more fundamental problem:

   f() {
     trap "echo ERR" ERR
     false
   }
   f

This is supposed to print ERR. You've implemented set +E (the default 
mode) as forcibly disabling ERR handlers inside functions, but that's 
not what it's supposed to do, nor what it's documented as doing in bash. 
What it's supposed to do is only to prevent outer ERR handlers from 
being inherited, it's not supposed to prevent ERR handlers defined 
within the function from taking effect.