Re: [PATCH] cli_hush: fix 'exit' cmd that was not exiting scripts

[Hector Palacios <hector.palacios@digi.com>]

On 11/21/22 09:55, Hector Palacios wrote:
> Hi Marek,
> 
> On 11/19/22 15:12, Marek Vasut wrote:
>> On 11/18/22 12:19, Hector Palacios wrote:
>>> Commit 8c4e3b79bd0bb76eea16869e9666e19047c0d005 supposedly
>>> passed one-level up the argument passed to 'exit' but it also
>>> broke 'exit' purpose of stopping a script.
>>>
>>> In reality, even if 'do_exit()' is capable of returning any
>>> integer, the cli only admits '1' or '0' as return values.
>>>
>>> This commit respects the current implementation to allow 'exit'
>>> to at least return '1' for future processing, but returns
>>> when the command being run is 'exit'.
>>>
>>> Before this:
>>>
>>>       => setenv foo 'echo bar ; exit 3 ; echo should not see this'; 
>>> run foo; echo $?
>>>       bar
>>>       should not see this
>>>       0
>>>       => setenv foo 'echo bar ; exit 1 ; echo should not see this'; 
>>> run foo; echo $?
>>>       bar
>>>       should not see this
>>>       0
>>>       => setenv foo 'echo bar ; exit 0 ; echo should not see this'; 
>>> run foo; echo $?
>>>       bar
>>>       should not see this
>>>       0
>>>       => setenv foo 'echo bar ; exit -1 ; echo should not see this'; 
>>> run foo; echo $?
>>>       bar
>>>       should not see this
>>>       0
>>>       => setenv foo 'echo bar ; exit -2 ; echo should not see this'; 
>>> run foo; echo $?
>>>       bar
>>>       should not see this
>>>       0
>>>       => setenv foo 'echo bar ; exit ; echo should not see this'; run 
>>> foo; echo $?
>>>       bar
>>>       should not see this
>>>       0
>>>
>>> After this:
>>>
>>>          => setenv foo 'echo bar ; exit 3 ; echo should not see 
>>> this'; run foo; echo $?
>>>          bar
>>>          1
>>>          => setenv foo 'echo bar ; exit 1 ; echo should not see 
>>> this'; run foo; echo $?
>>>          bar
>>>          1
>>>          => setenv foo 'echo bar ; exit 0 ; echo should not see 
>>> this'; run foo; echo $?
>>>          bar
>>>          0
>>>          => setenv foo 'echo bar ; exit -1 ; echo should not see 
>>> this'; run foo; echo $?
>>>          bar
>>>          0
>>>          => setenv foo 'echo bar ; exit -2 ; echo should not see 
>>> this'; run foo; echo $?
>>>          bar
>>>          0
>>>          => setenv foo 'echo bar ; exit ; echo should not see this'; 
>>> run foo; echo $?
>>>          bar
>>>          0
>>>
>>> Reported-by: Adrian Vovk <avovk@cc-sw.com>
>>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>>> ---
>>>   common/cli_hush.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/common/cli_hush.c b/common/cli_hush.c
>>> index 1467ff81b35b..9fe8b87e02d7 100644
>>> --- a/common/cli_hush.c
>>> +++ b/common/cli_hush.c
>>> @@ -1902,6 +1902,10 @@ static int run_list_real(struct pipe *pi)
>>>                       last_return_code = -rcode - 2;
>>>                       return -2;      /* exit */
>>>               }
>>> +             if (!strcmp(pi->progs->argv[0], "exit")) {
>>> +                     last_return_code = rcode;
>>> +                     return rcode;   /* exit */
>>> +             }
>>>               last_return_code=(rcode == 0) ? 0 : 1;
>>>   #endif
>>>   #ifndef __U_BOOT__
>>
>> Looking at the code just above this change 'if (rcode < -1)
>> last_return_code = -rcode - 2', that explains the odd 'return -r - 2' in
>> cmd/exit.c I think.
> 
> That's what I thought, too. The cli captures a -2 as the number to exit 
> a  script, and with -rcode -2 was exiting and returning a 0.
> Instead of capturing a magic number, I'm suggesting to capture 'exit' 
> command.
> 
> 
>> I wonder, can we somehow fix the return code handling in cmd/exit.c
>> instead, so that it would cover both this behavior listed in this patch,
>> and 8c4e3b79bd0 ("cmd: exit: Fix return value") ? The cmd/exit.c seems
>> like the right place to fix it.
> 
> I didn't revert or touched 8c4e3b79bd0 but if what you wanted to do with 
> that commit is to return any positive integer to the upper layers, I 
> must say that just doesn't work because the cli_hush only processes 1 
> (failure) or 0 (success), so there's no way for something such as 'exit 
> 3' to produce a $? of 3.
> I think the 'exit' command should only be used with this old U-Boot 
> standard of considering 1 a failure and 0 a success.
> 
> I could remove the 'if (rcode < -1)  last_return_code = -rcode - 2', 
> which doesn't add much value now, but other than that I'm unsure of what 
> you have in mind as to fix cmd/exit.c.

I just saw my patch causes a data abort on if conditionals, when 
accessing argv[0].

Maybe we'd rather simply revert 8c4e3b79bd0 ("cmd: exit: Fix return 
value") and let the exit command return 0 in all cases, as it is 
documented, at least until we find a proper solution.
-- 
Héctor Palacios