[PATCH] cli_hush: fix 'exit' cmd that was not exiting scripts
Hector Palacios
hector.palacios at digi.com
Mon Nov 21 17:32:04 CET 2022
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.
--
Héctor Palacios
More information about the U-Boot
mailing list