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

[Marek Vasut]

On 11/21/22 17:32, Hector Palacios wrote:
> 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 at cc-sw.com>
>>>> Signed-off-by: Hector Palacios <hector.palacios at 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.

We need to find a proper fix, no half-baked solutions. ( I need to think 
about the other email in this thread a bit more, I'll get back to you in 
a few days ).