[PATCH v2 1/2] cli: Correct several bugs in cli_getch()

[Simon Glass]

Hi Heinrich,

On Sun, 5 Feb 2023 at 14:29, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
>
> Am 5. Februar 2023 03:08:54 MEZ schrieb Simon Glass <sjg at chromium.org>:
> >This function does not behave as expected when unknown escape sequences
> >are sent to it:
> >
> >- it fails to store (and thus echo) the last character of the invalid
> >  sequence
>
> What would be the benefit of echoing it? Is there any reason to assume that we want an output for an escape sequence that we fail to parse?

Yes. In fact we don't even know if it is invalid or not. It is just
that U-Boot doesn't support it. The mimics the behaviour we used to
have, I believe.

Regards,
Simon



>
> Best regards
>
> Heinrich
>
> >- it fails to set esc_len to 0 when it finishes emitting the invalid
> >  sequence, meaning that the following character will appear to be part
> >  of a new escape sequence
> >- it processes the first character of the rejected sequence as a valid
> >  character, just starting the sequence all over again
> >
> >The last two bugs conspire to produce an "impossible condition #876"
> >message which is the main symptom of this behaviour.
> >
> >Fix these bugs and add a test to verify the behaviour.
> >
> >Signed-off-by: Simon Glass <sjg at chromium.org>
> >Reported-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> >---
> >
> >(no changes since v1)
> >
> > common/cli_getch.c   |  5 +++--
> > test/common/Makefile |  1 +
> > test/common/cread.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 52 insertions(+), 2 deletions(-)
> > create mode 100644 test/common/cread.c
> >
> >diff --git a/common/cli_getch.c b/common/cli_getch.c
> >index 87c23edcf4b..61d4cb261b8 100644
> >--- a/common/cli_getch.c
> >+++ b/common/cli_getch.c
> >@@ -129,7 +129,7 @@ static int cli_ch_esc(struct cli_ch_state *cch, int ichar,
> >
> >       *actp = act;
> >
> >-      return act == ESC_CONVERTED ? ichar : 0;
> >+      return ichar;
> > }
> >
> > int cli_ch_process(struct cli_ch_state *cch, int ichar)
> >@@ -145,6 +145,7 @@ int cli_ch_process(struct cli_ch_state *cch, int ichar)
> >                               return cch->esc_save[cch->emit_upto++];
> >                       cch->emit_upto = 0;
> >                       cch->emitting = false;
> >+                      cch->esc_len = 0;
> >               }
> >               return 0;
> >       } else if (ichar == -ETIMEDOUT) {
> >@@ -185,7 +186,7 @@ int cli_ch_process(struct cli_ch_state *cch, int ichar)
> >                       cch->esc_save[cch->esc_len++] = ichar;
> >                       ichar = cch->esc_save[cch->emit_upto++];
> >                       cch->emitting = true;
> >-                      break;
> >+                      return ichar;
> >               case ESC_CONVERTED:
> >                       /* valid escape sequence, return the resulting char */
> >                       cch->esc_len = 0;
> >diff --git a/test/common/Makefile b/test/common/Makefile
> >index cc918f64e54..a5ab10f6f69 100644
> >--- a/test/common/Makefile
> >+++ b/test/common/Makefile
> >@@ -3,3 +3,4 @@ obj-y += cmd_ut_common.o
> > obj-$(CONFIG_AUTOBOOT) += test_autoboot.o
> > obj-$(CONFIG_CYCLIC) += cyclic.o
> > obj-$(CONFIG_EVENT) += event.o
> >+obj-y += cread.o
> >diff --git a/test/common/cread.c b/test/common/cread.c
> >new file mode 100644
> >index 00000000000..3dce4bdb0ef
> >--- /dev/null
> >+++ b/test/common/cread.c
> >@@ -0,0 +1,48 @@
> >+// SPDX-License-Identifier: GPL-2.0+
> >+/*
> >+ * Copyright 2023 Google LLC
> >+ */
> >+
> >+#include <common.h>
> >+#include <cli.h>
> >+#include <test/common.h>
> >+#include <test/test.h>
> >+#include <test/ut.h>
> >+
> >+static int cli_ch_test(struct unit_test_state *uts)
> >+{
> >+      struct cli_ch_state s_cch, *cch = &s_cch;
> >+
> >+      cli_ch_init(cch);
> >+
> >+      /* should be nothing to return at first */
> >+      ut_asserteq(0, cli_ch_process(cch, 0));
> >+
> >+      /* check normal entry */
> >+      ut_asserteq('a', cli_ch_process(cch, 'a'));
> >+      ut_asserteq('b', cli_ch_process(cch, 'b'));
> >+      ut_asserteq('c', cli_ch_process(cch, 'c'));
> >+      ut_asserteq(0, cli_ch_process(cch, 0));
> >+
> >+      /* send an invalid escape sequence */
> >+      ut_asserteq(0, cli_ch_process(cch, '\e'));
> >+      ut_asserteq(0, cli_ch_process(cch, '['));
> >+
> >+      /*
> >+       * with the next char it sees that the sequence is invalid, so starts
> >+       * emitting it
> >+       */
> >+      ut_asserteq('\e', cli_ch_process(cch, 'X'));
> >+
> >+      /* now we set 0 bytes to empty the buffer */
> >+      ut_asserteq('[', cli_ch_process(cch, 0));
> >+      ut_asserteq('X', cli_ch_process(cch, 0));
> >+      ut_asserteq(0, cli_ch_process(cch, 0));
> >+
> >+      /* things are normal again */
> >+      ut_asserteq('a', cli_ch_process(cch, 'a'));
> >+      ut_asserteq(0, cli_ch_process(cch, 0));
> >+
> >+      return 0;
> >+}
> >+COMMON_TEST(cli_ch_test, 0);