On Fri, Oct 11, 2024 at 08:17:02PM +0200, Francesco Dolcini wrote: > Il 11 ottobre 2024 18:01:22 CEST, Massimiliano Minella > <[email protected]> ha scritto: > >Hello, > > > >On Tue, Oct 8, 2024 at 7:20 PM Francesco Dolcini <[email protected]> > >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 > >> > <[email protected]> napsal: > >> > > > >> > > From: Massimiliano Minella <[email protected]> > >> > > > >> > > 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 <[email protected]> > >> > > --- > >> > > 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
signature.asc
Description: PGP signature
