[PATCH v2] cmd: setexpr: fix no matching string in gsub return empty value
From: Massimiliano Minella massimiliano.minella@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@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; } else { - break; + debug("## MATCH ## %s\n", data); } + break; }
- debug("## MATCH ## %s\n", data); - if (!s) return 1;
@@ -540,7 +538,8 @@ U_BOOT_CMD( " - For each substring matching the regular expression <r> in the\n" " string <t>, substitute the string <s>. The result is\n" " assigned to <name>. If <t> is not supplied, use the old\n" - " value of <name>\n" + " value of <name>. If no substring matching <r> is found in <t>,\n" + " assign <t> to <name>.\n" "setexpr name sub r s [t]\n" " - Just like gsub(), but replace only the first matching substring" #endif diff --git a/doc/usage/cmd/setexpr.rst b/doc/usage/cmd/setexpr.rst index d245a13ca8..593a0ea91e 100644 --- a/doc/usage/cmd/setexpr.rst +++ b/doc/usage/cmd/setexpr.rst @@ -39,6 +39,7 @@ setexpr name gsub <r> <s> [<t>] string <t>, substitute the string <s>. The result is assigned to <name>. If <t> is not supplied, use the old value of <name>. + If no substring matching <r> is found in <t>, assign <t> to <name>.
setexpr name sub <r> <s> [<t>] Just like gsub(), but replace only the first matching substring diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c index 312593e1e3..ee329e94b8 100644 --- a/test/cmd/setexpr.c +++ b/test/cmd/setexpr.c @@ -179,6 +179,16 @@ static int setexpr_test_regex(struct unit_test_state *uts) val = env_get("mary"); ut_asserteq_str("this is a test", val);
+ /* No match */ + ut_assertok(run_command("setenv fred 'this is a test'", 0)); + ut_assertok(run_command("setenv mary ''", 0)); + ut_assertok(run_command("setexpr fred gsub us is "${fred}"", 0)); + ut_assertok(run_command("setexpr mary gsub us is "${fred}"", 0)); + val = env_get("fred"); + ut_asserteq_str("this is a test", val); + val = env_get("mary"); + ut_asserteq_str("this is a test", val); + unmap_sysmem(buf);
return 0;
On Thu, Feb 08, 2024 at 03:58:27PM +0100, Massimiliano Minella wrote:
From: Massimiliano Minella massimiliano.minella@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@se.com
Applied to u-boot/next, thanks!
HI,
čt 8. 2. 2024 v 16:00 odesílatel Massimiliano Minella massimiliano.minella@gmail.com napsal:
From: Massimiliano Minella massimiliano.minella@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@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.
Thanks, Michal
+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@gmail.com napsal:
From: Massimiliano Minella massimiliano.minella@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@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.
Francesco
Hello,
On Tue, Oct 8, 2024 at 7:20 PM Francesco Dolcini francesco@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@gmail.com napsal:
From: Massimiliano Minella massimiliano.minella@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@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?
Regards, Massimiliano
Il 11 ottobre 2024 18:01:22 CEST, Massimiliano Minella massimiliano.minella@gmail.com ha scritto:
Hello,
On Tue, Oct 8, 2024 at 7:20 PM Francesco Dolcini francesco@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@gmail.com napsal:
From: Massimiliano Minella massimiliano.minella@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@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
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@gmail.com ha scritto:
Hello,
On Tue, Oct 8, 2024 at 7:20 PM Francesco Dolcini francesco@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@gmail.com napsal:
From: Massimiliano Minella massimiliano.minella@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@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?
participants (4)
-
Francesco Dolcini -
Massimiliano Minella -
Michal Simek -
Tom Rini