cmd: exit: Exit functionality broken

[Marek Vasut]

On 12/13/22 16:41, Hector Palacios wrote:
> Hi Max,
> 
> On 12/13/22 13:24, Max van den Biggelaar wrote:
>> Hi,
>>
>> I have a question regarding the U-Boot exit command. We are currently 
>> using mainline U-Boot 2022.04 version to provide our embedded systems 
>> with a bootloader image. To start our firmware via U-Boot environment, 
>> we use a bootscript to start our firmware. However, when we tried to 
>> exit a bootscript with the exit command, the bootscript was never exited.
>>
>> After debugging to investigate the problem, we found this commit 
>> (https://github.com/u-boot/u-boot/commit/8c4e3b79bd0bb76eea16869e9666e19047c0d005) in mainline U-Boot:
>> cmd: exit: Fix return value
>>
>>
>> In case exit is called in a script without parameter, the command
>> returns -2 ; in case exit is called with a numerical parameter,
>> the command returns -2 and lower. This leads to the following problem:
>> => setenv foo 'echo bar ; exit 1' ; run foo ; echo $?
>> bar
>> 0
>> => setenv foo 'echo bar ; exit 0' ; run foo ; echo $?
>> bar
>> 0
>> => setenv foo 'echo bar ; exit -2' ; run foo ; echo $?
>> bar
>> 0
>> That is, no matter what the 'exit' command argument is, the return
>> value is always 0 and so it is not possible to use script return
>> value in subsequent tests.
>>
>> Fix this and simplify the exit command such that if exit is called with
>> no argument, the command returns 0, just like 'true' in cmd/test.c. In
>> case the command is called with any argument that is positive integer,
>> the argument is set as return value.
>> => setenv foo 'echo bar ; exit 1' ; run foo ; echo $?
>> bar
>> 1
>> => setenv foo 'echo bar ; exit 0' ; run foo ; echo $?
>> bar
>> 0
>> => setenv foo 'echo bar ; exit -2' ; run foo ; echo $?
>> bar
>> 0
>>
>> Note that this does change ABI established in 2004 , although it is
>> unclear whether that ABI was originally OK or not.
>>
>> Fixes: 
>> c26e454<https://github.com/u-boot/u-boot/commit/c26e454dfc6650428854fa2db3b1ed7f19e0ba0e>
>> Signed-off-by: Marek Vasut <marex at denx.de>
>> Cc: Pantelis Antoniou <pantelis.antoniou at konsulko.com>
>> Cc: Tom Rini <trini at konsulko.com>
>>
>> This commit does solve the problem of returning the correct value 
>> given to the exit command, but this breaks the following source code 
>> in common/cli_hush.c:
>> https://github.com/u-boot/u-boot/blob/master/common/cli_hush.c#L3207
>>
>> In the previous versions of U-Boot, such as 2020.04, the exit command 
>> returned -2, which was expected of the exit command API. However, 
>> after the patch above to fix the return value, -2 was never returned 
>> and the functionality to exit a bootscript or mainline U-Boot shell is 
>> not supported anymore. Thus, by the patch above the return value is 
>> fixed, but the functionality of the exit command is broken.
>>
>> My question is if the functionality of this patch is fully 
>> tested/qualified to push in mainline U-Boot source code? And if so, is 
>> the functionality of the exit command also going to be fixed so that 
>> in future U-Boot source code releases bootscripts can be exited with 
>> this command?
> 
> I believe Marek's commit must be reverted as having 'exit' return a code 
> other than 0 (success) or 1 (error) was never part of U-Boot. I don't 
> know if reverting may break newer scripts, but now many scripts are 
> broken because 'exit' does not currently work.
> 
> Anyway, Marek wanted to give it a second thought. See 
> https://www.mail-archive.com/u-boot%40lists.denx.de/msg456830.html

Right, reverting the aforementioned commit breaks exit return value 
handling. Special-casing 'exit' in shell with strcmp is not the right 
approach, the exit behavior clearly needs closer look. I just haven't 
had time to do that yet and reply to Hector.