[PATCH] menu: Ignore prompt variable if timeout is != 0

[PATCH] menu: Ignore prompt variable if timeout is != 0

[FUKAUMI Naoki]

From: Manuel Traut <manuel.traut@mt.com>

Since 739e8361f3fe78038251216df6096a32bc2d5839, a system with the following
/boot/extlinux/extlinux.conf (which sets timeout to 50) immediately boots the
first entry in the config without displaying a boot menu.  According to the
description, that should only happen if both prompt and timeout are set to zero
in the config, but it also happens with timeout set to a non-zero value.

Reported-by: Karsten Merker <merker@debian.org>
Signed-off-by: Manuel Traut <manuel.traut@mt.com>
---
 common/menu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/menu.c b/common/menu.c
index 8fe00965c0..8eabcccc87 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -277,6 +277,9 @@ int menu_get_choice(struct menu *m, void **choice)
 	if (!m->item_cnt)
 		return -ENOENT;
 
+	if (m->timeout)
+		return menu_interactive_choice(m, choice);
+
 	if (!m->prompt)
 		return menu_default_choice(m, choice);
 
-- 
2.39.0

Re: [PATCH] menu: Ignore prompt variable if timeout is != 0

[Jonas Karlman]

On 2023-07-14 09:36, FUKAUMI Naoki wrote:
> From: Manuel Traut <manuel.traut@mt.com>
> 
> Since 739e8361f3fe78038251216df6096a32bc2d5839, a system with the following
> /boot/extlinux/extlinux.conf (which sets timeout to 50) immediately boots the
> first entry in the config without displaying a boot menu.  According to the
> description, that should only happen if both prompt and timeout are set to zero
> in the config, but it also happens with timeout set to a non-zero value.
> 
> Reported-by: Karsten Merker <merker@debian.org>
> Signed-off-by: Manuel Traut <manuel.traut@mt.com>
> ---
>  common/menu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/common/menu.c b/common/menu.c
> index 8fe00965c0..8eabcccc87 100644
> --- a/common/menu.c
> +++ b/common/menu.c
> @@ -277,6 +277,9 @@ int menu_get_choice(struct menu *m, void **choice)
>  	if (!m->item_cnt)
>  		return -ENOENT;
>  
> +	if (m->timeout)
> +		return menu_interactive_choice(m, choice);

This should not be needed, if the user wants to prompt the menu there is
the PROMPT keyword that can be used in extlinux.conf, e.g.:

  PROMPT 1
  TIMEOUT 50

See https://wiki.archlinux.org/title/Syslinux#Boot_prompt

That should set pxe cfg->prompt = 1 and that in turn menu m->prompt = 1.

Regards,
Jonas

> +
>  	if (!m->prompt)
>  		return menu_default_choice(m, choice);
>  

Re: [PATCH] menu: Ignore prompt variable if timeout is != 0

[FUKAUMI Naoki]

hi,

thank you for your reply!

On 7/18/23 23:08, Jonas Karlman wrote:
> On 2023-07-14 09:36, FUKAUMI Naoki wrote:
>> From: Manuel Traut <manuel.traut@mt.com>
>>
>> Since 739e8361f3fe78038251216df6096a32bc2d5839, a system with the following
>> /boot/extlinux/extlinux.conf (which sets timeout to 50) immediately boots the
>> first entry in the config without displaying a boot menu.  According to the
>> description, that should only happen if both prompt and timeout are set to zero
>> in the config, but it also happens with timeout set to a non-zero value.
>>
>> Reported-by: Karsten Merker <merker@debian.org>
>> Signed-off-by: Manuel Traut <manuel.traut@mt.com>
>> ---
>>   common/menu.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/common/menu.c b/common/menu.c
>> index 8fe00965c0..8eabcccc87 100644
>> --- a/common/menu.c
>> +++ b/common/menu.c
>> @@ -277,6 +277,9 @@ int menu_get_choice(struct menu *m, void **choice)
>>   	if (!m->item_cnt)
>>   		return -ENOENT;
>>   
>> +	if (m->timeout)
>> +		return menu_interactive_choice(m, choice);
> 
> This should not be needed, if the user wants to prompt the menu there is
> the PROMPT keyword that can be used in extlinux.conf, e.g.:
> 
>    PROMPT 1
>    TIMEOUT 50
> 
> See https://wiki.archlinux.org/title/Syslinux#Boot_prompt
> 
> That should set pxe cfg->prompt = 1 and that in turn menu m->prompt = 1.

  https://source.denx.de/u-boot/u-boot/-/blob/master/common/menu.c#L346-351

this description is unclear for me if (timeout > 0) && (prompt == 0)

Best regards,

--
FUKAUMI Naoki
Radxa

> Regards,
> Jonas
> 
>> +
>>   	if (!m->prompt)
>>   		return menu_default_choice(m, choice);
>>   

Re: [PATCH] menu: Ignore prompt variable if timeout is != 0

[Traut Manuel LCPF-CH]

Hi,

> thank you for your reply! 
thanks for taking care on this somehow lost patch

> On 7/18/23 23:08, Jonas Karlman wrote:
>> On 2023-07-14 09:36, FUKAUMI Naoki wrote:
>>> From: Manuel Traut <manuel.traut@mt.com>
>>>
>>> Since 739e8361f3fe78038251216df6096a32bc2d5839, a system with the 
>>> following /boot/extlinux/extlinux.conf (which sets timeout to 50) 
>>> immediately boots the first entry in the config without displaying a 
>>> boot menu.  According to the description, that should only happen if 
>>> both prompt and timeout are set to zero in the config, but it also happens with timeout set to a non-zero value.
>>>
>>> Reported-by: Karsten Merker <merker@debian.org>
>>> Signed-off-by: Manuel Traut <manuel.traut@mt.com>
>>> ---
>>>   common/menu.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/common/menu.c b/common/menu.c index 
>>> 8fe00965c0..8eabcccc87 100644
>>> --- a/common/menu.c
>>> +++ b/common/menu.c
>>> @@ -277,6 +277,9 @@ int menu_get_choice(struct menu *m, void **choice)
>>>   	if (!m->item_cnt)
>>>   		return -ENOENT;
>>>   
>>> +	if (m->timeout)
>>> +		return menu_interactive_choice(m, choice);
>> 
>> This should not be needed, if the user wants to prompt the menu there 
>> is the PROMPT keyword that can be used in extlinux.conf, e.g.:
>> 
>>    PROMPT 1
>>    TIMEOUT 50
>> 
>> See https://wiki.archlinux.org/title/Syslinux#Boot_prompt
>> 
>> That should set pxe cfg->prompt = 1 and that in turn menu m->prompt = 1.
>
> https://source.denx.de/u-boot/u-boot/-/blob/master/common/menu.c#L346-351
>
> this description is unclear for me if (timeout > 0) && (prompt == 0)

This is my current understanding after reading the description multiple times:

            | timeout == 0 | timeout > 0
------------+--------------+-----------------------------------------
prompt == 0 | boot default | wait for timeout or user interrupt
------------+--------------+-----------------------------------------
prompt != 0 | ask user     | ask user

so for (timeout > 0) && (prompt == 0) I would expect to boot the default choice immediately.

Without reading the documentation I would expect
PROMPT 1
TIMEOUT 50
to show a prompt for 5 seconds and afterwards boot the default choice.
This also matches the current implantation and the explanation in the arch wiki.

The current implementation, behaves like this:

            | timeout == 0 | timeout > 0
------------+--------------+-----------------------------------------
prompt == 0 | boot default | boot default
------------+--------------+-----------------------------------------
prompt != 0 | ask user     | wait for timeout or user input

The patch under discussion considers a configuration of:
PROMPT 0
TIMEOUT 50
that currently is booting the default target immediately.
This clearly does not match the description.

The patch would change the behavior like this:

            | timeout == 0 | timeout > 0
------------+--------------+-----------------------------------------
prompt == 0 | boot default | wait for timeout or user input
------------+--------------+-----------------------------------------
prompt != 0 | ask user     | wait for timeout or user input

It does not match the description regarding:
"prompt - If 1, the user will be prompted for input regardless of the value of timeout"

If I write a configuration like:
PROMPT 1
TIMEOUT 50
I would expect to get a prompt for 5 seconds, than boot the default target.

I can update the patch to include changing the documentation along with table above as a comment to make it easier understandable.
Or shall I update the patch to match the behavior described in the first table?
Just let me know..

Regards
Manuel

Re: [PATCH] menu: Ignore prompt variable if timeout is != 0

[Jonas Karlman]

On 2023-07-19 10:16, Traut Manuel LCPF-CH wrote:
> Hi,
> 
>> thank you for your reply! 
> thanks for taking care on this somehow lost patch
> 
>> On 7/18/23 23:08, Jonas Karlman wrote:
>>> On 2023-07-14 09:36, FUKAUMI Naoki wrote:
>>>> From: Manuel Traut <manuel.traut@mt.com>
>>>>
>>>> Since 739e8361f3fe78038251216df6096a32bc2d5839, a system with the 
>>>> following /boot/extlinux/extlinux.conf (which sets timeout to 50) 
>>>> immediately boots the first entry in the config without displaying a 
>>>> boot menu.  According to the description, that should only happen if 
>>>> both prompt and timeout are set to zero in the config, but it also happens with timeout set to a non-zero value.
>>>>
>>>> Reported-by: Karsten Merker <merker@debian.org>
>>>> Signed-off-by: Manuel Traut <manuel.traut@mt.com>
>>>> ---
>>>>   common/menu.c | 3 +++
>>>>   1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/common/menu.c b/common/menu.c index 
>>>> 8fe00965c0..8eabcccc87 100644
>>>> --- a/common/menu.c
>>>> +++ b/common/menu.c
>>>> @@ -277,6 +277,9 @@ int menu_get_choice(struct menu *m, void **choice)
>>>>   	if (!m->item_cnt)
>>>>   		return -ENOENT;
>>>>   
>>>> +	if (m->timeout)
>>>> +		return menu_interactive_choice(m, choice);
>>>
>>> This should not be needed, if the user wants to prompt the menu there 
>>> is the PROMPT keyword that can be used in extlinux.conf, e.g.:
>>>
>>>    PROMPT 1
>>>    TIMEOUT 50
>>>
>>> See https://wiki.archlinux.org/title/Syslinux#Boot_prompt
>>>
>>> That should set pxe cfg->prompt = 1 and that in turn menu m->prompt = 1.
>>
>> https://source.denx.de/u-boot/u-boot/-/blob/master/common/menu.c#L346-351
>>
>> this description is unclear for me if (timeout > 0) && (prompt == 0)
> 
> This is my current understanding after reading the description multiple times:
> 
>             | timeout == 0 | timeout > 0
> ------------+--------------+-----------------------------------------
> prompt == 0 | boot default | wait for timeout or user interrupt
> ------------+--------------+-----------------------------------------
> prompt != 0 | ask user     | ask user
> 
> so for (timeout > 0) && (prompt == 0) I would expect to boot the default choice immediately.
> 
> Without reading the documentation I would expect
> PROMPT 1
> TIMEOUT 50
> to show a prompt for 5 seconds and afterwards boot the default choice.
> This also matches the current implantation and the explanation in the arch wiki.
> 
> The current implementation, behaves like this:
> 
>             | timeout == 0 | timeout > 0
> ------------+--------------+-----------------------------------------
> prompt == 0 | boot default | boot default
> ------------+--------------+-----------------------------------------
> prompt != 0 | ask user     | wait for timeout or user input
> 
> The patch under discussion considers a configuration of:
> PROMPT 0
> TIMEOUT 50
> that currently is booting the default target immediately.
> This clearly does not match the description.
> 
> The patch would change the behavior like this:
> 
>             | timeout == 0 | timeout > 0
> ------------+--------------+-----------------------------------------
> prompt == 0 | boot default | wait for timeout or user input
> ------------+--------------+-----------------------------------------
> prompt != 0 | ask user     | wait for timeout or user input
> 
> It does not match the description regarding:
> "prompt - If 1, the user will be prompted for input regardless of the value of timeout"
> 
> If I write a configuration like:
> PROMPT 1
> TIMEOUT 50
> I would expect to get a prompt for 5 seconds, than boot the default target.
> 
> I can update the patch to include changing the documentation along with table above as a comment to make it easier understandable.
> Or shall I update the patch to match the behavior described in the first table?
> Just let me know..

I agree that the documentation is not clear, because this is a common
menu function I would expect the use of timeout and prompt to be
explicit and that each control a single function.

prompt: 0 = do not ask user, 1 = ask user
timeout: 0 = wait indefinitely, > 0 time to wait for input

If anything needs to be done related to how extlinux.conf handles menu
options the menu config should probably be changed in pxe_utils.c and
not in the common menu.c.

My expectations related to extlinux.conf would be:
- No need to prompt or wait for timeout if there is only one bootable
  choice, regardless of PROMPT or TIMEOUT values
- Should not prompt when I have explicitly specified PROMPT 0
- Having multiple bootable choices and a TIMEOUT > 0 or PROMPT 1 should
  prompt, at least if I have some way to see and control the menu

Regards,
Jonas

> 
> Regards
> Manuel

AW: EXTERNAL - [PATCH] menu: Ignore prompt variable if timeout is != 0

[Traut Manuel LCPF-CH]

Hi,

>>>>> Since 739e8361f3fe78038251216df6096a32bc2d5839, a system with the 
>>>>> following /boot/extlinux/extlinux.conf (which sets timeout to 50) 
>>>>> immediately boots the first entry in the config without displaying 
>>>>> a boot menu.  According to the description, that should only happen 
>>>>> if both prompt and timeout are set to zero in the config, but it also happens with timeout set to a non-zero value.
>>>>>
>>>>> Reported-by: Karsten Merker <merker@debian.org>
>>>>> Signed-off-by: Manuel Traut <manuel.traut@mt.com>
>>>>> ---
>>>>>   common/menu.c | 3 +++
>>>>>   1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/common/menu.c b/common/menu.c index
>>>>> 8fe00965c0..8eabcccc87 100644
>>>>> --- a/common/menu.c
>>>>> +++ b/common/menu.c
>>>>> @@ -277,6 +277,9 @@ int menu_get_choice(struct menu *m, void **choice)
>>>>>   	if (!m->item_cnt)
>>>>>   		return -ENOENT;
>>>>>   
>>>>> +	if (m->timeout)
>>>>> +		return menu_interactive_choice(m, choice);
>>>>
>>>> This should not be needed, if the user wants to prompt the menu 
>>>> there is the PROMPT keyword that can be used in extlinux.conf, e.g.:
>>>>
>>>>    PROMPT 1
>>>>    TIMEOUT 50
>>>>
>>>> See https://wiki.archlinux.org/title/Syslinux#Boot_prompt
>>>>
>>>> That should set pxe cfg->prompt = 1 and that in turn menu m->prompt = 1.
>>>
>>> https://source.denx.de/u-boot/u-boot/-/blob/master/common/menu.c#L346
>>> -351
>>>
>>> this description is unclear for me if (timeout > 0) && (prompt == 0)
>> 
>> This is my current understanding after reading the description multiple times:
>> 
>>             | timeout == 0 | timeout > 0
>> ------------+--------------+-----------------------------------------
>> prompt == 0 | boot default | wait for timeout or user interrupt
>> ------------+--------------+-----------------------------------------
>> prompt != 0 | ask user     | ask user
>> 
>> so for (timeout > 0) && (prompt == 0) I would expect to boot the default choice immediately.
>> 
>> Without reading the documentation I would expect PROMPT 1 TIMEOUT 50 
>> to show a prompt for 5 seconds and afterwards boot the default choice.
>> This also matches the current implantation and the explanation in the arch wiki.
>> 
>> The current implementation, behaves like this:
>> 
>>             | timeout == 0 | timeout > 0
>> ------------+--------------+-----------------------------------------
>> prompt == 0 | boot default | boot default
>> ------------+--------------+-----------------------------------------
>> prompt != 0 | ask user     | wait for timeout or user input
>> 
>> The patch under discussion considers a configuration of:
>> PROMPT 0
>> TIMEOUT 50
>> that currently is booting the default target immediately.
>> This clearly does not match the description.
>> 
>> The patch would change the behavior like this:
>> 
>>             | timeout == 0 | timeout > 0
>> ------------+--------------+-----------------------------------------
>> prompt == 0 | boot default | wait for timeout or user input
>> ------------+--------------+-----------------------------------------
>> prompt != 0 | ask user     | wait for timeout or user input
>> 
>> It does not match the description regarding:
>> "prompt - If 1, the user will be prompted for input regardless of the value of timeout"
>> 
>> If I write a configuration like:
>> PROMPT 1
>> TIMEOUT 50
>> I would expect to get a prompt for 5 seconds, than boot the default target.
>> 
>> I can update the patch to include changing the documentation along with table above as a comment to make it easier understandable.
>> Or shall I update the patch to match the behavior described in the first table?
>> Just let me know..
>
> I agree that the documentation is not clear, because this is a common menu function I would expect the use of timeout and prompt to be explicit and that each control a single function.
>
> prompt: 0 = do not ask user, 1 = ask user
> timeout: 0 = wait indefinitely, > 0 time to wait for input

This should match the current implementation (but not documentation..)

@FUKAUMI Naoki, @merker@debian.org - is it ok for you if we keep this behavior and I will fix the documentation?

> If anything needs to be done related to how extlinux.conf handles menu options the menu config should probably be changed in pxe_utils.c and not in the common menu.c.
>
> My expectations related to extlinux.conf would be:
> - No need to prompt or wait for timeout if there is only one bootable
>   choice, regardless of PROMPT or TIMEOUT values
> - Should not prompt when I have explicitly specified PROMPT 0
> - Having multiple bootable choices and a TIMEOUT > 0 or PROMPT 1 should
>   prompt, at least if I have some way to see and control the menu

This is also fine for me, we could implement this as an additional feature, if we have agreed on the basic behavior.

Best regards
Manuel

Re: AW: EXTERNAL - [PATCH] menu: Ignore prompt variable if timeout is != 0

[FUKAUMI Naoki]

hi,

On 7/25/23 17:23, Traut Manuel LCPF-CH wrote:
> Hi,
> 
>>>>>> Since 739e8361f3fe78038251216df6096a32bc2d5839, a system with the
>>>>>> following /boot/extlinux/extlinux.conf (which sets timeout to 50)
>>>>>> immediately boots the first entry in the config without displaying
>>>>>> a boot menu.  According to the description, that should only happen
>>>>>> if both prompt and timeout are set to zero in the config, but it also happens with timeout set to a non-zero value.
>>>>>>
>>>>>> Reported-by: Karsten Merker <merker@debian.org>
>>>>>> Signed-off-by: Manuel Traut <manuel.traut@mt.com>
>>>>>> ---
>>>>>>    common/menu.c | 3 +++
>>>>>>    1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/common/menu.c b/common/menu.c index
>>>>>> 8fe00965c0..8eabcccc87 100644
>>>>>> --- a/common/menu.c
>>>>>> +++ b/common/menu.c
>>>>>> @@ -277,6 +277,9 @@ int menu_get_choice(struct menu *m, void **choice)
>>>>>>    	if (!m->item_cnt)
>>>>>>    		return -ENOENT;
>>>>>>    
>>>>>> +	if (m->timeout)
>>>>>> +		return menu_interactive_choice(m, choice);
>>>>>
>>>>> This should not be needed, if the user wants to prompt the menu
>>>>> there is the PROMPT keyword that can be used in extlinux.conf, e.g.:
>>>>>
>>>>>     PROMPT 1
>>>>>     TIMEOUT 50
>>>>>
>>>>> See https://wiki.archlinux.org/title/Syslinux#Boot_prompt
>>>>>
>>>>> That should set pxe cfg->prompt = 1 and that in turn menu m->prompt = 1.
>>>>
>>>> https://source.denx.de/u-boot/u-boot/-/blob/master/common/menu.c#L346
>>>> -351
>>>>
>>>> this description is unclear for me if (timeout > 0) && (prompt == 0)
>>>
>>> This is my current understanding after reading the description multiple times:
>>>
>>>              | timeout == 0 | timeout > 0
>>> ------------+--------------+-----------------------------------------
>>> prompt == 0 | boot default | wait for timeout or user interrupt
>>> ------------+--------------+-----------------------------------------
>>> prompt != 0 | ask user     | ask user
>>>
>>> so for (timeout > 0) && (prompt == 0) I would expect to boot the default choice immediately.
>>>
>>> Without reading the documentation I would expect PROMPT 1 TIMEOUT 50
>>> to show a prompt for 5 seconds and afterwards boot the default choice.
>>> This also matches the current implantation and the explanation in the arch wiki.
>>>
>>> The current implementation, behaves like this:
>>>
>>>              | timeout == 0 | timeout > 0
>>> ------------+--------------+-----------------------------------------
>>> prompt == 0 | boot default | boot default
>>> ------------+--------------+-----------------------------------------
>>> prompt != 0 | ask user     | wait for timeout or user input
>>>
>>> The patch under discussion considers a configuration of:
>>> PROMPT 0
>>> TIMEOUT 50
>>> that currently is booting the default target immediately.
>>> This clearly does not match the description.
>>>
>>> The patch would change the behavior like this:
>>>
>>>              | timeout == 0 | timeout > 0
>>> ------------+--------------+-----------------------------------------
>>> prompt == 0 | boot default | wait for timeout or user input
>>> ------------+--------------+-----------------------------------------
>>> prompt != 0 | ask user     | wait for timeout or user input
>>>
>>> It does not match the description regarding:
>>> "prompt - If 1, the user will be prompted for input regardless of the value of timeout"
>>>
>>> If I write a configuration like:
>>> PROMPT 1
>>> TIMEOUT 50
>>> I would expect to get a prompt for 5 seconds, than boot the default target.
>>>
>>> I can update the patch to include changing the documentation along with table above as a comment to make it easier understandable.
>>> Or shall I update the patch to match the behavior described in the first table?
>>> Just let me know..
>>
>> I agree that the documentation is not clear, because this is a common menu function I would expect the use of timeout and prompt to be explicit and that each control a single function.
>>
>> prompt: 0 = do not ask user, 1 = ask user
>> timeout: 0 = wait indefinitely, > 0 time to wait for input
> 
> This should match the current implementation (but not documentation..)
> 
> @FUKAUMI Naoki, @merker@debian.org - is it ok for you if we keep this behavior and I will fix the documentation?

I'm fine.

>> If anything needs to be done related to how extlinux.conf handles menu options the menu config should probably be changed in pxe_utils.c and not in the common menu.c.
>>
>> My expectations related to extlinux.conf would be:
>> - No need to prompt or wait for timeout if there is only one bootable
>>    choice, regardless of PROMPT or TIMEOUT values
>> - Should not prompt when I have explicitly specified PROMPT 0
>> - Having multiple bootable choices and a TIMEOUT > 0 or PROMPT 1 should
>>    prompt, at least if I have some way to see and control the menu
> 
> This is also fine for me, we could implement this as an additional feature, if we have agreed on the basic behavior.

I like this too.

Best regards,

--
FUKAUMI Naoki
Radxa

> Best regards
> Manuel