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

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

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.


> 
> btw. it would be good to write a unit test for this, since it is
> becoming messy.

Regards
-- 
Héctor Palacios