| Message ID | 20210527212404.38250-1-farhan.ali@broadcom.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Sean Anderson |
| Headers | show |
| Series | common: The do_repeat flag interferes with commands issued via run_command API | expand |
> 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; >
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
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
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;
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(+)