Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

[Roland Gaudig (OSS)]

Dear Wolfgang,

On Wed, 23. Jun 2021 08:03, Wolfgang Denk <wd@denx.de> wrote:
> 
> Dear Roland,
> 
> In message <20210622135042.133904-2-roland.gaudig-oss@weidmueller.com> you 
> wrote:
>>
>> This patch extends the setexpr command with a dec operator to
>> convert an input value to decimal.
> ...
>> +     /* hexadecimal to decimal conversion: "setexpr name dec value" */
>> +     if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
>> +             w = cmd_get_data_size(argv[3], 4);
>> +             a = get_arg(argv[3], w);
>> +             return env_set_ulong(argv[1], a);
>> +     }
> 
> Should there not be a test for 4 arguments and the third _not_ being
> "dec" ?  Like "setexpr foo hex 42" ?

Yes it's possible to add further conversion operations. But I didn't saw a
need for hex in the first place, as hex is currently the default within
U-Boot. But when adding a decimal prefix as Simon proposed, hex conversion
becomes necessary.

Best regards,
Roland Gaudig

Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

[Wolfgang Denk]

Dear Roland,

In message <d057362e-b317-0361-92b0-b8b88ee38e3c@weidmueller.com> you wrote:
>
> > ...
> >> +     /* hexadecimal to decimal conversion: "setexpr name dec value" */
> >> +     if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
> >> +             w = cmd_get_data_size(argv[3], 4);
> >> +             a = get_arg(argv[3], w);
> >> +             return env_set_ulong(argv[1], a);
> >> +     }
> > 
> > Should there not be a test for 4 arguments and the third _not_ being
> > "dec" ?  Like "setexpr foo hex 42" ?
>
> Yes it's possible to add further conversion operations. But I didn't saw a
> need for hex in the first place, as hex is currently the default within

You misunderstand.  With your code, the incorect command "setexpr foo hex 42"
would not raise any error message.

Assume such a call:

	setexpr foo kjkjkjlkj 42

This should raise an error, right?


Instead of

	if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
		...
	}

you would need something like:

	if (argc == 4) {
		if strcmp((argv[2], "dec") != 0) {
			print error message
			bail our
		}
		...
	}

But see my other suggestion anyway - why add just decimal format
when using sprintf() with a format string allows for all kinds of
fancy uses?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"Love is an ideal thing, marriage a real thing; a  confusion  of  the
real with the ideal never goes unpunished."                  - Goethe

Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Wed, Jun 23, 2021 at 08:08:50AM +0200, Wolfgang Denk wrote:
> Dear Simon,
> 
> In message <CAPnjgZ13=2SCsZC1Y+m-HZOiBhpGiEFe2dRWwK9sAZHutgaO7Q@mail.gmail.com> you wrote:
> >
> > >  > 0m123 ? ('m' for deciMal).
> > >
> > > Perhaps 0d123? Though I would prefer to remove many of the implicit
> > > assumptions of hex input.
> >
> > Right, we can't use 'd' because it is valid hex.
> >
> > I believe hex is the right default. We just need an easy way to use decimal.
> 
> Maybe we should make this more general and support an even wirder
> range of formats?  Instead of just converting to decimal, we could
> pass a format string for sprintf() ?
> 
> Like:
> 
> 	# setexpr foo fmt %d $value

I like this idea as well.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

[Roland Gaudig (OSS)]

Hi Simon,

On Tue, 22. Jun 2021 at 21:25, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Roland,
> 
> On Tue, 22 Jun 2021 at 07:51, <roland.gaudig-oss@weidmueller.com> wrote:
>>
>> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>>
>> This patch extends the setexpr command with a dec operator to
>> convert an input value to decimal.
>>
>> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
>> ---
>>
>>  cmd/setexpr.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/cmd/setexpr.c b/cmd/setexpr.c
>> index e828be3970..2d4bee2182 100644
>> --- a/cmd/setexpr.c
>> +++ b/cmd/setexpr.c
>> @@ -370,15 +370,16 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int 
> flag, int argc,
>>         int w;
>>
>>         /*
>> -        * We take 3, 5, or 6 arguments:
>> +        * We take 3, 4, 5, or 6 arguments:
>>          * 3 : setexpr name value
>> +        * 4 : setexpr name dec value
>>          * 5 : setexpr name val1 op val2
>>          *     setexpr name [g]sub r s
>>          * 6 : setexpr name [g]sub r s t
>>          */
>>
>>         /* > 6 already tested by max command args */
>> -       if ((argc < 3) || (argc == 4))
>> +       if (argc < 3)
>>                 return CMD_RET_USAGE;
>>
>>         w = cmd_get_data_size(argv[0], 4);
>> @@ -398,6 +399,13 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, 
> int argc,
>>                 return ret;
>>         }
>>
>> +       /* hexadecimal to decimal conversion: "setexpr name dec value" */
>> +       if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
>> +               w = cmd_get_data_size(argv[3], 4);
>> +               a = get_arg(argv[3], w);
>> +               return env_set_ulong(argv[1], a);
>> +       }
>> +
>>         /* 5 or 6 args (6 args only with [g]sub) */
>>  #ifdef CONFIG_REGEX
>>         /*
>> @@ -515,4 +523,8 @@ U_BOOT_CMD(
>>         "setexpr name sub r s [t]\n"
>>         "    - Just like gsub(), but replace only the first matching 
> substring"
>>  #endif
>> +       "\n"
>> +       "setexpr name dec [*]value\n"
>> +       "    - set environment variable 'name' to the result of the 
> decimal\n"
>> +       "      conversion of [*]value.\n"
>>  );
>> --
>> 2.25.1
>>
> 
> This seems reasonable to me.
> 
> I have been thinking of introducing a prefix for decimal, perhaps
> 0m123 ? ('m' for deciMal).

This 0x prefix would then also be parsed by get_arg(), as it does currently
with the 0x prefix?

For my application as input to the Linux bootargs it is important, that the
resulting text stored in the variable after running setexpr has no prefix
at all.

> Can you please add a test for this in test//cmd/setexpr.c and also,
> how about adding something in doc/usage?

Yes, I will add a test and a description to doc/usage.

Regards,
Roland

Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ13=2SCsZC1Y+m-HZOiBhpGiEFe2dRWwK9sAZHutgaO7Q@mail.gmail.com> you wrote:
>
> >  > 0m123 ? ('m' for deciMal).
> >
> > Perhaps 0d123? Though I would prefer to remove many of the implicit
> > assumptions of hex input.
>
> Right, we can't use 'd' because it is valid hex.
>
> I believe hex is the right default. We just need an easy way to use decimal.

Maybe we should make this more general and support an even wirder
range of formats?  Instead of just converting to decimal, we could
pass a format string for sprintf() ?

Like:

	# setexpr foo fmt %d $value

?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Question: How does one get fresh air into a Russian church?
Answer:   One clicks on an icon, and a window opens!

Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

[Wolfgang Denk]

Dear Roland,

In message <20210622135042.133904-2-roland.gaudig-oss@weidmueller.com> you wrote:
>
> This patch extends the setexpr command with a dec operator to
> convert an input value to decimal.
...
> +	/* hexadecimal to decimal conversion: "setexpr name dec value" */
> +	if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
> +		w = cmd_get_data_size(argv[3], 4);
> +		a = get_arg(argv[3], w);
> +		return env_set_ulong(argv[1], a);
> +	}

Should there not be a test for 4 arguments and the third _not_ being
"dec" ?  Like "setexpr foo hex 42" ?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
A wise man asks himself the reason for his  mistakes,  while  a  fool
will ask others.

Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

[Simon Glass]

Hi Sean,

On Tue, 22 Jun 2021 at 13:30, Sean Anderson <sean.anderson@seco.com> wrote:
>
>
>
> On 6/22/21 3:25 PM, Simon Glass wrote:
>  > Hi Roland,
>  >
>  > On Tue, 22 Jun 2021 at 07:51, <roland.gaudig-oss@weidmueller.com> wrote:
>  >>
>  >> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>  >>
>  >> This patch extends the setexpr command with a dec operator to
>  >> convert an input value to decimal.
>  >>
>  >> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
>  >> ---
>  >>
>  >>  cmd/setexpr.c | 16 ++++++++++++++--
>  >>  1 file changed, 14 insertions(+), 2 deletions(-)
>  >>
>  >> diff --git a/cmd/setexpr.c b/cmd/setexpr.c
>  >> index e828be3970..2d4bee2182 100644
>  >> --- a/cmd/setexpr.c
>  >> +++ b/cmd/setexpr.c
>  >> @@ -370,15 +370,16 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
>  >>         int w;
>  >>
>  >>         /*
>  >> -        * We take 3, 5, or 6 arguments:
>  >> +        * We take 3, 4, 5, or 6 arguments:
>  >>          * 3 : setexpr name value
>  >> +        * 4 : setexpr name dec value
>  >>          * 5 : setexpr name val1 op val2
>  >>          *     setexpr name [g]sub r s
>  >>          * 6 : setexpr name [g]sub r s t
>  >>          */
>  >>
>  >>         /* > 6 already tested by max command args */
>  >> -       if ((argc < 3) || (argc == 4))
>  >> +       if (argc < 3)
>  >>                 return CMD_RET_USAGE;
>  >>
>  >>         w = cmd_get_data_size(argv[0], 4);
>  >> @@ -398,6 +399,13 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
>  >>                 return ret;
>  >>         }
>  >>
>  >> +       /* hexadecimal to decimal conversion: "setexpr name dec value" */
>  >> +       if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
>  >> +               w = cmd_get_data_size(argv[3], 4);
>  >> +               a = get_arg(argv[3], w);
>  >> +               return env_set_ulong(argv[1], a);
>  >> +       }
>  >> +
>  >>         /* 5 or 6 args (6 args only with [g]sub) */
>  >>  #ifdef CONFIG_REGEX
>  >>         /*
>  >> @@ -515,4 +523,8 @@ U_BOOT_CMD(
>  >>         "setexpr name sub r s [t]\n"
>  >>         "    - Just like gsub(), but replace only the first matching substring"
>  >>  #endif
>  >> +       "\n"
>  >> +       "setexpr name dec [*]value\n"
>  >> +       "    - set environment variable 'name' to the result of the decimal\n"
>  >> +       "      conversion of [*]value.\n"
>  >>  );
>  >> --
>  >> 2.25.1
>  >>
>  >
>  > This seems reasonable to me.
>  >
>  > I have been thinking of introducing a prefix for decimal, perhaps
>  > 0m123 ? ('m' for deciMal).
>
> Perhaps 0d123? Though I would prefer to remove many of the implicit
> assumptions of hex input.

Right, we can't use 'd' because it is valid hex.

I believe hex is the right default. We just need an easy way to use decimal.


- Simon

>
> --Sean
>
>  >
>  > Can you please add a test for this in test//cmd/setexpr.c and also,
>  > how about adding something in doc/usage?
>  >
>  > Regards,
>  > Simon
>  >

Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

[Sean Anderson]

On 6/22/21 3:25 PM, Simon Glass wrote:
 > Hi Roland,
 >
 > On Tue, 22 Jun 2021 at 07:51, <roland.gaudig-oss@weidmueller.com> wrote:
 >>
 >> From: Roland Gaudig <roland.gaudig@weidmueller.com>
 >>
 >> This patch extends the setexpr command with a dec operator to
 >> convert an input value to decimal.
 >>
 >> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
 >> ---
 >>
 >>  cmd/setexpr.c | 16 ++++++++++++++--
 >>  1 file changed, 14 insertions(+), 2 deletions(-)
 >>
 >> diff --git a/cmd/setexpr.c b/cmd/setexpr.c
 >> index e828be3970..2d4bee2182 100644
 >> --- a/cmd/setexpr.c
 >> +++ b/cmd/setexpr.c
 >> @@ -370,15 +370,16 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 >>         int w;
 >>
 >>         /*
 >> -        * We take 3, 5, or 6 arguments:
 >> +        * We take 3, 4, 5, or 6 arguments:
 >>          * 3 : setexpr name value
 >> +        * 4 : setexpr name dec value
 >>          * 5 : setexpr name val1 op val2
 >>          *     setexpr name [g]sub r s
 >>          * 6 : setexpr name [g]sub r s t
 >>          */
 >>
 >>         /* > 6 already tested by max command args */
 >> -       if ((argc < 3) || (argc == 4))
 >> +       if (argc < 3)
 >>                 return CMD_RET_USAGE;
 >>
 >>         w = cmd_get_data_size(argv[0], 4);
 >> @@ -398,6 +399,13 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 >>                 return ret;
 >>         }
 >>
 >> +       /* hexadecimal to decimal conversion: "setexpr name dec value" */
 >> +       if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
 >> +               w = cmd_get_data_size(argv[3], 4);
 >> +               a = get_arg(argv[3], w);
 >> +               return env_set_ulong(argv[1], a);
 >> +       }
 >> +
 >>         /* 5 or 6 args (6 args only with [g]sub) */
 >>  #ifdef CONFIG_REGEX
 >>         /*
 >> @@ -515,4 +523,8 @@ U_BOOT_CMD(
 >>         "setexpr name sub r s [t]\n"
 >>         "    - Just like gsub(), but replace only the first matching substring"
 >>  #endif
 >> +       "\n"
 >> +       "setexpr name dec [*]value\n"
 >> +       "    - set environment variable 'name' to the result of the decimal\n"
 >> +       "      conversion of [*]value.\n"
 >>  );
 >> --
 >> 2.25.1
 >>
 >
 > This seems reasonable to me.
 >
 > I have been thinking of introducing a prefix for decimal, perhaps
 > 0m123 ? ('m' for deciMal).

Perhaps 0d123? Though I would prefer to remove many of the implicit
assumptions of hex input.

--Sean

 >
 > Can you please add a test for this in test//cmd/setexpr.c and also,
 > how about adding something in doc/usage?
 >
 > Regards,
 > Simon
 >

Re: [PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

[Simon Glass]

Hi Roland,

On Tue, 22 Jun 2021 at 07:51, <roland.gaudig-oss@weidmueller.com> wrote:
>
> From: Roland Gaudig <roland.gaudig@weidmueller.com>
>
> This patch extends the setexpr command with a dec operator to
> convert an input value to decimal.
>
> Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
> ---
>
>  cmd/setexpr.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/cmd/setexpr.c b/cmd/setexpr.c
> index e828be3970..2d4bee2182 100644
> --- a/cmd/setexpr.c
> +++ b/cmd/setexpr.c
> @@ -370,15 +370,16 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
>         int w;
>
>         /*
> -        * We take 3, 5, or 6 arguments:
> +        * We take 3, 4, 5, or 6 arguments:
>          * 3 : setexpr name value
> +        * 4 : setexpr name dec value
>          * 5 : setexpr name val1 op val2
>          *     setexpr name [g]sub r s
>          * 6 : setexpr name [g]sub r s t
>          */
>
>         /* > 6 already tested by max command args */
> -       if ((argc < 3) || (argc == 4))
> +       if (argc < 3)
>                 return CMD_RET_USAGE;
>
>         w = cmd_get_data_size(argv[0], 4);
> @@ -398,6 +399,13 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
>                 return ret;
>         }
>
> +       /* hexadecimal to decimal conversion: "setexpr name dec value" */
> +       if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
> +               w = cmd_get_data_size(argv[3], 4);
> +               a = get_arg(argv[3], w);
> +               return env_set_ulong(argv[1], a);
> +       }
> +
>         /* 5 or 6 args (6 args only with [g]sub) */
>  #ifdef CONFIG_REGEX
>         /*
> @@ -515,4 +523,8 @@ U_BOOT_CMD(
>         "setexpr name sub r s [t]\n"
>         "    - Just like gsub(), but replace only the first matching substring"
>  #endif
> +       "\n"
> +       "setexpr name dec [*]value\n"
> +       "    - set environment variable 'name' to the result of the decimal\n"
> +       "      conversion of [*]value.\n"
>  );
> --
> 2.25.1
>

This seems reasonable to me.

I have been thinking of introducing a prefix for decimal, perhaps
0m123 ? ('m' for deciMal).

Can you please add a test for this in test//cmd/setexpr.c and also,
how about adding something in doc/usage?

Regards,
Simon

[PATCH v1 1/1] cmd: setexpr: add dec operation for converting variable to decimal

[roland.gaudig-oss]

From: Roland Gaudig <roland.gaudig@weidmueller.com>

This patch extends the setexpr command with a dec operator to
convert an input value to decimal.

Signed-off-by: Roland Gaudig <roland.gaudig@weidmueller.com>
---

 cmd/setexpr.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/cmd/setexpr.c b/cmd/setexpr.c
index e828be3970..2d4bee2182 100644
--- a/cmd/setexpr.c
+++ b/cmd/setexpr.c
@@ -370,15 +370,16 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 	int w;
 
 	/*
-	 * We take 3, 5, or 6 arguments:
+	 * We take 3, 4, 5, or 6 arguments:
 	 * 3 : setexpr name value
+	 * 4 : setexpr name dec value
 	 * 5 : setexpr name val1 op val2
 	 *     setexpr name [g]sub r s
 	 * 6 : setexpr name [g]sub r s t
 	 */
 
 	/* > 6 already tested by max command args */
-	if ((argc < 3) || (argc == 4))
+	if (argc < 3)
 		return CMD_RET_USAGE;
 
 	w = cmd_get_data_size(argv[0], 4);
@@ -398,6 +399,13 @@ static int do_setexpr(struct cmd_tbl *cmdtp, int flag, int argc,
 		return ret;
 	}
 
+	/* hexadecimal to decimal conversion: "setexpr name dec value" */
+	if (argc == 4 && (strcmp(argv[2], "dec") == 0)) {
+		w = cmd_get_data_size(argv[3], 4);
+		a = get_arg(argv[3], w);
+		return env_set_ulong(argv[1], a);
+	}
+
 	/* 5 or 6 args (6 args only with [g]sub) */
 #ifdef CONFIG_REGEX
 	/*
@@ -515,4 +523,8 @@ U_BOOT_CMD(
 	"setexpr name sub r s [t]\n"
 	"    - Just like gsub(), but replace only the first matching substring"
 #endif
+	"\n"
+	"setexpr name dec [*]value\n"
+	"    - set environment variable 'name' to the result of the decimal\n"
+	"      conversion of [*]value.\n"
 );
-- 
2.25.1