Re: [PATCH v13 15/24] cli: add modern hush as parser for run_command*()

[Francis Laniel <francis.laniel@amarulasolutions.com>]

Hi!

Le mardi 26 décembre 2023, 10:46:48 CET Simon Glass a écrit :
> Hi,
> 
> On Fri, Dec 22, 2023 at 9:23 PM Tom Rini <trini@konsulko.com> wrote:
> > On Fri, Dec 22, 2023 at 10:10:42PM +0100, Francis Laniel wrote:
> > > Hi!
> > > 
> > > Le vendredi 22 décembre 2023, 22:02:35 CET Francis Laniel a écrit :
> > > > Enables using, in code, modern hush as parser for run_command function
> > > > family. It also enables the command run to be used by CLI user of
> > > > modern
> > > > hush.
> > > > 
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > Signed-off-by: Francis Laniel <francis.laniel@amarulasolutions.com>
> > 
> > [snip]
> > 
> > > > diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
> > > > index a9b555c779..104f49deef 100644
> > > > --- a/test/boot/bootflow.c
> > > > +++ b/test/boot/bootflow.c
> > > > @@ -710,7 +710,21 @@ static int bootflow_scan_menu_boot(struct
> > > > unit_test_state *uts) ut_assert_skip_to_line("(2 bootflows, 2
> > > > valid)");
> > > > 
> > > >     ut_assert_nextline("Selected: Armbian");
> > > > 
> > > > -   ut_assert_skip_to_line("Boot failed (err=-14)");
> > > > +
> > > > +   if (gd->flags & GD_FLG_HUSH_OLD_PARSER) {
> > > > +           /*
> > > > +            * With old hush, despite booti failing to boot, i.e.
> > > > returning
> > > > +            * CMD_RET_FAILURE, run_command() returns 0 which leads
> > > 
> > > bootflow_boot(),
> > > 
> > > > as +                 * we are using bootmeth_script here, to return
> > > > -EFAULT. +            */
> > > > +           ut_assert_skip_to_line("Boot failed (err=-14)");
> > > > +   } else if (gd->flags & GD_FLG_HUSH_MODERN_PARSER) {
> > > > +           /*
> > > > +            * While with modern one, run_command() propagates
> > > 
> > > CMD_RET_FAILURE
> > > 
> > > > returned +           * by booti, so we get 1 here.
> > > > +            */
> > > > +           ut_assert_skip_to_line("Boot failed (err=1)");
> > > > +   }
> > > 
> > > I would like to give a bit of context here.
> > > With the following patch:
> > > diff --git a/test/py/tests/test_ut.py b/test/py/tests/test_ut.py
> > > index c169c835e3..cc5adda0a3 100644
> > > --- a/test/py/tests/test_ut.py
> > > +++ b/test/py/tests/test_ut.py
> > > @@ -173,7 +173,7 @@ else
> > > 
> > >         fi
> > >  
> > >  fi
> > >  booti ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r}
> > > 
> > > -
> > > +echo $?
> > > 
> > >  # Recompile with:
> > >  # mkimage -C none -A arm -T script -d /boot/boot.cmd /boot/boot.scr
> > >  ''' % (mmc_dev)
> > > 
> > > We can easily see that booti is failing while running the test:
> > > $ ./test/py/test.py -o log_cli=true -s --build -v -k
> > > 'test_ut[ut_bootstd_bootflow_scan_menu_boot'
> > > ...
> > > Aborting!
> > > Failed to load '/boot/dtb/rockchip/overlay/-fixup.scr'
> > > 1
> > > 
> > > The problem with old hush, is that the 1 returned here, which
> > > corresponds to CMD_RET_FAILURE, is not propagated as the return value
> > > of run_command(). So, this lead the -EFAULT branch here to be taken:
> > > int bootflow_boot(struct bootflow *bflow)
> > > {
> > > 
> > >       /* ... */
> > >       
> > >       ret = bootmeth_boot(bflow->method, bflow);
> > >       if (ret)
> > >       
> > >               return log_msg_ret("boot", ret);
> > >       
> > >       /*
> > >       
> > >        * internal error, should not get here since we should have booted
> > >        * something or returned an error
> > >        */
> > >       
> > >       return log_msg_ret("end", -EFAULT);
> > > 
> > > }
> > > 
> > > With modern hush, CMD_RET_FAILURE is propagated as the return value of
> > > run_command().
> > > As a consequence, we return with log_mst_ret("boot", 1), which leaded to
> > > this test to fail.
> > > The above modification consists in adapting the expected output to the
> > > current shell flavor.
> > > I think this is the good thing to do, as I find modern hush behavior
> > > better
> > > than the old one, i.e. it propagates CMD_RET_FAILURE as return of
> > > run_command().
> > 
> > Oh very nice, thanks for digging in to this and explaining!
> 
> Yes, thank you from me too!

You are welcome!
 
> 
> - Simon

Best regards and have a nice end of the year!