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 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?

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