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

[Francesco Dolcini]

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.

Francesco