All of lore.kernel.org
 help / color / mirror / Atom feed
* cmd: exit: Exit functionality broken
@ 2022-12-13 12:24 Max van den Biggelaar
  2022-12-13 15:41 ` Hector Palacios
  0 siblings, 1 reply; 4+ messages in thread
From: Max van den Biggelaar @ 2022-12-13 12:24 UTC (permalink / raw)
  To: u-boot, marex; +Cc: Martijn de Gouw

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@denx.de>
Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Cc: Tom Rini <trini@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?

Thank you in advance for the response.

Met vriendelijke groet / Kind regards,

Max van den Biggelaar
Designer

Mobile  +31 62 57 28 396
Phone   +31 40 26 76 200
Address         Science Park Eindhoven 5501
5692 EM SON, The Netherlands

www.prodrive-technologies.com<http://www.prodrive-technologies.com>

Disclaimer: The content of this e-mail is intended solely for the use of the Individual or entity to whom it is addressed. If you have received this communication in error, be aware that forwarding it, copying it, or in any way disclosing its content to any other person, is strictly prohibited. If you have received this communication in error, please notify the author by replying to this e-mail immediately.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: cmd: exit: Exit functionality broken
  2022-12-13 12:24 cmd: exit: Exit functionality broken Max van den Biggelaar
@ 2022-12-13 15:41 ` Hector Palacios
  2022-12-13 16:06   ` Marek Vasut
  0 siblings, 1 reply; 4+ messages in thread
From: Hector Palacios @ 2022-12-13 15:41 UTC (permalink / raw)
  To: Max van den Biggelaar, u-boot, marex; +Cc: Martijn de Gouw

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@denx.de>
> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> Cc: Tom Rini <trini@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

Regards
-- 
Héctor Palacios


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: cmd: exit: Exit functionality broken
  2022-12-13 15:41 ` Hector Palacios
@ 2022-12-13 16:06   ` Marek Vasut
  2022-12-14  8:55     ` Max van den Biggelaar
  0 siblings, 1 reply; 4+ messages in thread
From: Marek Vasut @ 2022-12-13 16:06 UTC (permalink / raw)
  To: Hector Palacios, Max van den Biggelaar, u-boot; +Cc: Martijn de Gouw

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@denx.de>
>> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> Cc: Tom Rini <trini@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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: cmd: exit: Exit functionality broken
  2022-12-13 16:06   ` Marek Vasut
@ 2022-12-14  8:55     ` Max van den Biggelaar
  0 siblings, 0 replies; 4+ messages in thread
From: Max van den Biggelaar @ 2022-12-14  8:55 UTC (permalink / raw)
  To: Marek Vasut, Hector Palacios, u-boot; +Cc: Martijn de Gouw

Hi Hector and Marek,

Thanks for your reply. I am glad that there is already a thread open for this problem. As reverting the commit seems to be the best way to resolve this problem, we will take the same approach in our forked U-Boot mainline source code.

I am looking forward to the solution in mainline U-Boot. Thanks for your support.

Met vriendelijke groet / Kind regards,

Max van den Biggelaar
Designer

Mobile  +31 62 57 28 396
Phone   +31 40 26 76 200
Address         Science Park Eindhoven 5501
5692 EM SON, The Netherlands

www.prodrive-technologies.com<http://www.prodrive-technologies.com>

Disclaimer: The content of this e-mail is intended solely for the use of the Individual or entity to whom it is addressed. If you have received this communication in error, be aware that forwarding it, copying it, or in any way disclosing its content to any other person, is strictly prohibited. If you have received this communication in error, please notify the author by replying to this e-mail immediately.
________________________________
From: Marek Vasut <marex@denx.de>
Sent: Tuesday, December 13, 2022 5:06 PM
To: Hector Palacios <hector.palacios@digi.com>; Max van den Biggelaar <max.van.den.biggelaar@prodrive-technologies.com>; u-boot@lists.denx.de <u-boot@lists.denx.de>
Cc: Martijn de Gouw <martijn.de.gouw@prodrive-technologies.com>
Subject: Re: cmd: exit: Exit functionality broken

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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fu-boot%2Fu-boot%2Fcommit%2F8c4e3b79bd0bb76eea16869e9666e19047c0d005&amp;data=05%7C01%7Cmax.van.den.biggelaar%40prodrive-technologies.com%7Cdc9f555b4f9047280cb508dadd23f512%7C612607c95af74e7f8976faf1ae77be60%7C0%7C0%7C638065443753164173%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=nhHtOiLzPhWV0VXIQhXS5mvefORmuierzfOV%2B69o10A%3D&amp;reserved=0) 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fu-boot%2Fu-boot%2Fcommit%2Fc26e454dfc6650428854fa2db3b1ed7f19e0ba0e&amp;data=05%7C01%7Cmax.van.den.biggelaar%40prodrive-technologies.com%7Cdc9f555b4f9047280cb508dadd23f512%7C612607c95af74e7f8976faf1ae77be60%7C0%7C0%7C638065443753164173%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=WPDkviE3%2Bw%2BVOb4AQ34byFp99Ovi18fTGR%2F8C4t0SHo%3D&amp;reserved=0>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> Cc: Tom Rini <trini@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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fu-boot%2Fu-boot%2Fblob%2Fmaster%2Fcommon%2Fcli_hush.c%23L3207&amp;data=05%7C01%7Cmax.van.den.biggelaar%40prodrive-technologies.com%7Cdc9f555b4f9047280cb508dadd23f512%7C612607c95af74e7f8976faf1ae77be60%7C0%7C0%7C638065443753164173%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=iZMaTL7J477g9JKwiI0%2BmLurEmNzcl4rhTEoF0WPlNs%3D&amp;reserved=0
>>
>> 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.mail-archive.com%2Fu-boot%2540lists.denx.de%2Fmsg456830.html&amp;data=05%7C01%7Cmax.van.den.biggelaar%40prodrive-technologies.com%7Cdc9f555b4f9047280cb508dadd23f512%7C612607c95af74e7f8976faf1ae77be60%7C0%7C0%7C638065443753164173%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=t1obkYm9Zye5BSsip8PVkwYwMdPj8kt0h5LBieJajNc%3D&amp;reserved=0

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-12-14 10:39 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-13 12:24 cmd: exit: Exit functionality broken Max van den Biggelaar
2022-12-13 15:41 ` Hector Palacios
2022-12-13 16:06   ` Marek Vasut
2022-12-14  8:55     ` Max van den Biggelaar

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.