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. Francesco
