* [PATCH] cli_hush: fix 'exit' cmd that was not exiting scripts
@ 2022-11-18 11:19 Hector Palacios
2022-11-19 14:12 ` Marek Vasut
0 siblings, 1 reply; 5+ messages in thread
From: Hector Palacios @ 2022-11-18 11:19 UTC (permalink / raw)
To: u-boot; +Cc: marex, festevam, Adrian Vovk
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__
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] cli_hush: fix 'exit' cmd that was not exiting scripts
2022-11-18 11:19 [PATCH] cli_hush: fix 'exit' cmd that was not exiting scripts Hector Palacios
@ 2022-11-19 14:12 ` Marek Vasut
2022-11-21 8:55 ` Hector Palacios
0 siblings, 1 reply; 5+ messages in thread
From: Marek Vasut @ 2022-11-19 14:12 UTC (permalink / raw)
To: Hector Palacios, u-boot; +Cc: festevam, Adrian Vovk
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.
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.
btw. it would be good to write a unit test for this, since it is
becoming messy.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cli_hush: fix 'exit' cmd that was not exiting scripts
2022-11-19 14:12 ` Marek Vasut
@ 2022-11-21 8:55 ` Hector Palacios
2022-11-21 16:32 ` Hector Palacios
0 siblings, 1 reply; 5+ messages in thread
From: Hector Palacios @ 2022-11-21 8:55 UTC (permalink / raw)
To: Marek Vasut, u-boot; +Cc: festevam, Adrian Vovk
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cli_hush: fix 'exit' cmd that was not exiting scripts
2022-11-21 8:55 ` Hector Palacios
@ 2022-11-21 16:32 ` Hector Palacios
2022-11-25 21:35 ` Marek Vasut
0 siblings, 1 reply; 5+ messages in thread
From: Hector Palacios @ 2022-11-21 16:32 UTC (permalink / raw)
To: Marek Vasut, u-boot; +Cc: festevam
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cli_hush: fix 'exit' cmd that was not exiting scripts
2022-11-21 16:32 ` Hector Palacios
@ 2022-11-25 21:35 ` Marek Vasut
0 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2022-11-25 21:35 UTC (permalink / raw)
To: Hector Palacios, u-boot; +Cc: festevam
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@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.
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 ).
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-25 21:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-18 11:19 [PATCH] cli_hush: fix 'exit' cmd that was not exiting scripts Hector Palacios
2022-11-19 14:12 ` Marek Vasut
2022-11-21 8:55 ` Hector Palacios
2022-11-21 16:32 ` Hector Palacios
2022-11-25 21:35 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.