[PATCH v2] cmd: setexpr: fix no matching string in gsub return empty value

[Tom Rini]

On Fri, Oct 11, 2024 at 08:17:02PM +0200, Francesco Dolcini wrote:
> Il 11 ottobre 2024 18:01:22 CEST, Massimiliano Minella <massimiliano.minella at gmail.com> ha scritto:
> >Hello,
> >
> >On Tue, Oct 8, 2024 at 7:20 PM Francesco Dolcini <francesco at dolcini.it> wrote:
> >>
> >> +Tom
> >>
> >> Hello Massimiliano,
> >>
> >> On Tue, Sep 03, 2024 at 01:06:21PM +0200, Michal Simek wrote:
> >> > HI,
> >> >
> >> > čt 8. 2. 2024 v 16:00 odesílatel Massimiliano Minella
> >> > <massimiliano.minella at gmail.com> napsal:
> >> > >
> >> > > From: Massimiliano Minella <massimiliano.minella at se.com>
> >> > >
> >> > > In gsub, when the destination string is empty, the string 't' is
> >> > > provided and the regular expression doesn't match, then the final result
> >> > > is an empty string.
> >> > >
> >> > > Example:
> >> > >
> >> > > => echo ${foo}
> >> > >
> >> > > => setenv foo
> >> > > => setexpr foo gsub e a bar
> >> > > => echo ${foo}
> >> > >
> >> > > =>
> >> > >
> >> > > The variable ${foo} should contain "bar" and the lack of match shouldn't
> >> > > be considered an error.
> >> > >
> >> > > This patch fixes the erroneous behavior by removing the return
> >> > > statement and breaking out of the loop in case of lack of match.
> >> > >
> >> > > Also add a test for the no match case.
> >> > >
> >> > > Signed-off-by: Massimiliano Minella <massimiliano.minella at se.com>
> >> > > ---
> >> > > Changes in V2:
> >> > >  - update documentation to describe the behavior
> >> > >
> >> > >  cmd/setexpr.c             |  9 ++++-----
> >> > >  doc/usage/cmd/setexpr.rst |  1 +
> >> > >  test/cmd/setexpr.c        | 10 ++++++++++
> >> > >  3 files changed, 15 insertions(+), 5 deletions(-)
> >> > >
> >> > > diff --git a/cmd/setexpr.c b/cmd/setexpr.c
> >> > > index 233471f6cb..ab76824a32 100644
> >> > > --- a/cmd/setexpr.c
> >> > > +++ b/cmd/setexpr.c
> >> > > @@ -216,14 +216,12 @@ int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
> >> > >                 if (res == 0) {
> >> > >                         if (loop == 0) {
> >> > >                                 debug("%s: No match\n", data);
> >> > > -                               return 1;
> >> >
> >> > This patch actually changed the return value from command.
> >> > In board/xilinx/zynqmp/zynqmp_kria.env we have
> >> >
> >> > bootcmd=setenv model $board_name && if setexpr model gsub
> >> > .*$k24_starter* $k24_starter || setexpr model gsub .*$k26_starter*
> >> > $k26_starter; then run som_cc_boot; else run som_mmc_boot; run
> >> > som_cc_boot; fi
> >> >
> >> > and this patch actually breaked it because we rely on return value.
> >> > Changing return value is not described and I want to know if this
> >> > patch should be fixed
> >> > or we should update our commands to match new return value.
> >>
> >> Massimilano, any comment on this?
> >>
> >> This change broke also our use case - our board is no longer booting
> >> with v2024.10 U-Boot. I do not think we should change something like
> >> that without a clear reason and I was not able to understand it from the
> >> commit message.
> >
> >The reason why I proposed the patch was that I found the behavior of setexpr
> >gsub not consistent with the behavior of the same function in gawk, which,
> >according to the commit message that introduced gsub/sub, both commands are
> >closely modeled after. Also I didn't see a particular reason for returning an
> >empty string or throwing an error when doing a string substitution that provides
> >no match, so IMHO it was a bug.
> >
> >From a quick grep on the master branch it seems that the command is used only in
> >board/xilinx/zynqmp/zynqmp_kria.env and include/env/ti/ti_common.env. In the
> >first I see it has already been fixed and in the second the change has no
> >impact.
> >
> >It's unfortunate that it broke the boot of your board, maybe something as it has
> >been done for the zynqmp_kria is feasible according to your use of the command?
> 
> The U-Boot commands are used (also) outside of the U-Boot repository. In my case it was a boot script that was relying on the previous return code.
> 
> Of course, I can adapt the script to work with the new semantic. And I have also to consider that my boot script might be used on an older U-Boot, so in practice I need to handle both, and I did it.
> 
> To me it was a bad decision to change the semantic here. My own issue is solved, as I wrote, but I would advise to just revert it.

Would you be able to write an update to doc/usage/cmd/setexpr.rst that
spells out our deviations from gawk gsub?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241016/d09e13f5/attachment.sig>