[BUG] Re: [PATCH v3 02/25] cli: Move readline character-processing to a state machine

[Simon Glass]

Hi Heinrich,

On Tue, 24 Jan 2023 at 08:19, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 1/6/23 15:52, Simon Glass wrote:
> > The current cread_line() function is very long. It handles the escape
> > processing inline. The menu command does similar processing but at the
> > character level, so there is some duplication.
> >
> > Split the character processing into a new function cli_ch_process() which
> > processes individual characters and returns the resulting input character,
> > taking account of escape sequences. It requires the caller to set up and
> > maintain its state.
> >
> > Update cread_line() to use this new function.
> >
> > The only intended functional change is that an invalid escape sequence
> > does not add invalid/control characters into the input buffer, but instead
> > discards these.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
>
> Hello Simon
>
> in the Change Boot Order menu of the eficonfig command I hit the
> PAGE-UP or PAGE-DOWN button and get this output:
>
>        [ ]  label0038
>        [ ]  label0039impossible condition #876
> impossible condition #876
>    Press UP/DOWN to move, +/- to change orde
>    Press SPACE to activate or deactivate the entry
>    Select [Save] to complete, ESC/CTRL+C to quit
>
> Hitting an unsupported key should not result in output.
>
> This is the line providing the output.
>
> common/cli_getch.c:201:
> puts("impossible condition #876\n");
>
> How line be reached by an "impossible condition". That text does not
> make any sense.
>
> This is the patch to blame
>
> b08e9d4b6632 ("cli: Move readline character-processing to a state machine")
>
> Please, remove that debug message.

This was already in the code. I just split it out into a separate file.

>
> The conitrace command shows these escape sequences:
>
> PAGE-DOWN 1b 5b 36 7e
> PAGE-UP 1b 5b 35 7e

I believe it indicates that the sequence is not understood. It is
therefore rejected and sent out as an escape sequence to the terminal.

This works by calling cli_ch_process() with a character of 0, to find
out if there is a rejected sequence to be emitted. Only once it
returns 0 should getchar() be called. See the detailed function
comments for how it works.

>From what I can see, bootmenu_autoboot_loop() is violating that.
Probably that is what is going on. The message is correct in that the
condition cannot happen if the caller is behaving correctly.

In any case it seems that these keys are not supported, so in addition
to fixing the bug you found, we need to add support for them to
cli_ch_esc().


>
> Best regards
>
> Heinrich
>

Regards,
Simon