[PATCH] common: The do_repeat flag interferes with commands issued via run_command API

[PATCH] common: The do_repeat flag interferes with commands issued via run_command API

[Farhan Ali]

Must clear the do_repeat flag once it is consumed.

Signed-off-by: Farhan Ali <farhan.ali@broadcom.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Sean Anderson <seanga2@gmail.com>
Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
Cc: Farhan Ali <farhan.ali@broadcom.com>
Cc: "peng.wang@smartm.com" <peng.wang@smartm.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>

---
 common/cli_hush.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/common/cli_hush.c b/common/cli_hush.c
index 1467ff81b3..1c9adf5683 100644
--- a/common/cli_hush.c
+++ b/common/cli_hush.c
@@ -1559,6 +1559,11 @@ static int run_pipe_real(struct pipe *pi)
 # endif
 #endif	/* __U_BOOT__ */
 
+	/* Clear do_repeat after consumption, avoids conflicts
+	 * with cmds issued  via run_command API
+	 */
+	do_repeat = 0;
+
 	nextin = 0;
 #ifndef __U_BOOT__
 	pi->pgrp = -1;
-- 
2.27.0

Re: [PATCH] common: The do_repeat flag interferes with commands issued via run_command API

[Sean Anderson]

> Re: [PATCH] common: The do_repeat flag interferes with commands issued via run_command API

The tag here should be "hush: ..." The subject should be an action,
such as "Clear do_repeat flag after running commands".

On 5/27/21 5:24 PM, Farhan Ali wrote:
> Must clear the do_repeat flag once it is consumed.

What is "it" here? Please add a few more sentences describing why you
want to change this. For example, you could note that do_repeat is a
file-level variable which is used by get_user_input to signal when a
command should be repeated. Though, I wonder why we don't set the flags
in the first place...

> 
> Signed-off-by: Farhan Ali <farhan.ali@broadcom.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Sean Anderson <seanga2@gmail.com>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Cc: Farhan Ali <farhan.ali@broadcom.com>
> Cc: "peng.wang@smartm.com" <peng.wang@smartm.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> 
> ---
>   common/cli_hush.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/common/cli_hush.c b/common/cli_hush.c
> index 1467ff81b3..1c9adf5683 100644
> --- a/common/cli_hush.c
> +++ b/common/cli_hush.c
> @@ -1559,6 +1559,11 @@ static int run_pipe_real(struct pipe *pi)
>   # endif
>   #endif	/* __U_BOOT__ */
>   
> +	/* Clear do_repeat after consumption, avoids conflicts

Multi-line comments should start with a blank line.

> +	 * with cmds issued  via run_command API
> +	 */
> +	do_repeat = 0;
> +

Can you add a test case for this?

--Sean

>   	nextin = 0;
>   #ifndef __U_BOOT__
>   	pi->pgrp = -1;
> 

Re: [PATCH] common: The do_repeat flag interferes with commands issued via run_command API

[Farhan Ali]

[-- Attachment #1.1: Type: text/plain, Size: 2780 bytes --]

Hi Sean,

Thanks for taking a look. I will add the changes you requested. However a
test case might be difficult to add in the automated tests. This problem
only happens if there is an asynchronous event ( packet received/Button
pressed ) which triggers a run_command API call AND when the user has
entered some console commands which have enabled the do_repeat flag. If the
do_repeat flag is set AND the command issued via the run_command API is a
'non-repeatable' command (e.g mmc write), the command is ignored.

On Thu, May 27, 2021 at 4:24 PM Sean Anderson <seanga2@gmail.com> wrote:

> > Re: [PATCH] common: The do_repeat flag interferes with commands issued
> via run_command API
>
> The tag here should be "hush: ..." The subject should be an action,
> such as "Clear do_repeat flag after running commands".
>
> On 5/27/21 5:24 PM, Farhan Ali wrote:
> > Must clear the do_repeat flag once it is consumed.
>
> What is "it" here? Please add a few more sentences describing why you
> want to change this. For example, you could note that do_repeat is a
> file-level variable which is used by get_user_input to signal when a
> command should be repeated. Though, I wonder why we don't set the flags
> in the first place...
>
> 'it' refers to the do_repeat flag.

> >
> > Signed-off-by: Farhan Ali <farhan.ali@broadcom.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Sean Anderson <seanga2@gmail.com>
> > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> > Cc: Farhan Ali <farhan.ali@broadcom.com>
> > Cc: "peng.wang@smartm.com" <peng.wang@smartm.com>
> > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> >
> > ---
> >   common/cli_hush.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/common/cli_hush.c b/common/cli_hush.c
> > index 1467ff81b3..1c9adf5683 100644
> > --- a/common/cli_hush.c
> > +++ b/common/cli_hush.c
> > @@ -1559,6 +1559,11 @@ static int run_pipe_real(struct pipe *pi)
> >   # endif
> >   #endif      /* __U_BOOT__ */
> >
> > +     /* Clear do_repeat after consumption, avoids conflicts
>
> Multi-line comments should start with a blank line.
>
> > +      * with cmds issued  via run_command API
> > +      */
> > +     do_repeat = 0;
> > +
>
> Can you add a test case for this?
>
> So a use case could be as follows:
(1) User issues several repeated commands on console. This sets do_repeat
flag internally
(2) Webserver embedded in main polling loop of uboot detects an image
upgrade request
(3) Image downloads, about to be written via run_command("mmc write etc.")
(4) Since 'mmc write' is a non-repeatable command, issuing it with
do_repeat flag set results in command getting ignored
(5) Image upgrade fails

> --Sean
>
> >       nextin = 0;
> >   #ifndef __U_BOOT__
> >       pi->pgrp = -1;
> >
>
>

Regards,

Farhan

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4203 bytes --]

Re: [PATCH] common: The do_repeat flag interferes with commands issued via run_command API

[Sean Anderson]

On 5/28/21 2:34 PM, Farhan Ali wrote:
> Hi Sean,
> 
> Thanks for taking a look. I will add the changes you requested.
> However a test case might be difficult to add in the automated tests.
> This problem only happens if there is an asynchronous event ( packet

What do you mean by an "asynchronous event"?

> received/Button pressed ) which triggers a run_command API call AND
> when the user has entered some console commands which have enabled the
> do_repeat flag. If the do_repeat flag is set AND the command issued
> via the run_command API is a 'non-repeatable' command (e.g mmc write),
> the command is ignored.

Ok, so if I understand correctly, this occurs when someone uses a
repeatable command which also calls run_command. Perhaps you can trigger
this behavior with a python test?

> 
> On Thu, May 27, 2021 at 4:24 PM Sean Anderson <seanga2@gmail.com <mailto:seanga2@gmail.com>> wrote:
> 
>      > Re: [PATCH] common: The do_repeat flag interferes with commands issued via run_command API
> 
>     The tag here should be "hush: ..." The subject should be an action,
>     such as "Clear do_repeat flag after running commands".
> 
>     On 5/27/21 5:24 PM, Farhan Ali wrote:
>      > Must clear the do_repeat flag once it is consumed.
> 
>     What is "it" here? Please add a few more sentences describing why you
>     want to change this. For example, you could note that do_repeat is a
>     file-level variable which is used by get_user_input to signal when a
>     command should be repeated. Though, I wonder why we don't set the flags
>     in the first place...
> 
> 'it' refers to the do_repeat flag.

I found this wording confusing because the flag itself is not really
"consumed", per se. Perhaps "used" is a better verb?

> 
>      >
>      > Signed-off-by: Farhan Ali <farhan.ali@broadcom.com <mailto:farhan.ali@broadcom.com>>
>      > Cc: Simon Glass <sjg@chromium.org <mailto:sjg@chromium.org>>
>      > Cc: Sean Anderson <seanga2@gmail.com <mailto:seanga2@gmail.com>>
>      > Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk <mailto:rasmus.villemoes@prevas.dk>>
>      > Cc: Farhan Ali <farhan.ali@broadcom.com <mailto:farhan.ali@broadcom.com>>
>      > Cc: "peng.wang@smartm.com <mailto:peng.wang@smartm.com>" <peng.wang@smartm.com <mailto:peng.wang@smartm.com>>
>      > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com <mailto:patrick.delaunay@foss.st.com>>
>      >
>      > ---
>      >   common/cli_hush.c | 5 +++++
>      >   1 file changed, 5 insertions(+)
>      >
>      > diff --git a/common/cli_hush.c b/common/cli_hush.c
>      > index 1467ff81b3..1c9adf5683 100644
>      > --- a/common/cli_hush.c
>      > +++ b/common/cli_hush.c
>      > @@ -1559,6 +1559,11 @@ static int run_pipe_real(struct pipe *pi)
>      >   # endif
>      >   #endif      /* __U_BOOT__ */
>      >
>      > +     /* Clear do_repeat after consumption, avoids conflicts
> 
>     Multi-line comments should start with a blank line.
> 
>      > +      * with cmds issued  via run_command API
>      > +      */
>      > +     do_repeat = 0;
>      > +
> 
>     Can you add a test case for this?
> 
> So a use case could be as follows:
> (1) User issues several repeated commands on console. This sets do_repeat flag internally
> (2) Webserver embedded in main polling loop of uboot detects an image upgrade request
> (3) Image downloads, about to be written via run_command("mmc write etc.")
> (4) Since 'mmc write' is a non-repeatable command, issuing it with do_repeat flag set results in command getting ignored
> (5) Image upgrade fails
> 
>     --Sean
> 
>      >       nextin = 0;
>      >   #ifndef __U_BOOT__
>      >       pi->pgrp = -1;
>      >
> 
> 
> 
> Regards,
> 
> Farhan