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 at debian.org>
>>>>> Signed-off-by: Manuel Traut <manuel.traut at 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 at 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