Patchwork [U-Boot,2/8] Add setenv_uint() and setenv_addr()

login
register
mail settings
Submitter Simon Glass
Date Oct. 22, 2011, 4:51 a.m.
Message ID <1319259100-11376-3-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/121136/
State New, archived
Headers show

Comments

Simon Glass - Oct. 22, 2011, 4:51 a.m.
It seems we put numbers and addresses into environment variables a lot.
We should have some functions to do this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 common/cmd_nvedit.c |   29 +++++++++++++++++++++++++++++
 include/common.h    |    2 ++
 2 files changed, 31 insertions(+), 0 deletions(-)
Mike Frysinger - Oct. 23, 2011, 5:29 a.m.
On Sat, Oct 22, 2011 at 00:51, Simon Glass <sjg@chromium.org> wrote:
> +int setenv_ulong(const char *varname, ulong value)
> +{
> +       char *str = simple_itoa(value);
> +
> +       return setenv(varname, str);
> +}

could be a one liner, but works either way

> +int setenv_addr(const char *varname, const void *addr)
> +{
> +       char str[17];

char str[sizeof(addr) * 2 + 1];

> +       sprintf(str, "%x", (uintptr_t)addr);

i wonder if we should use %p and drop the cast
-mike
Simon Glass - Oct. 25, 2011, 9:35 p.m.
Hi Mike

On Sat, Oct 22, 2011 at 10:29 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Sat, Oct 22, 2011 at 00:51, Simon Glass <sjg@chromium.org> wrote:
>> +int setenv_ulong(const char *varname, ulong value)
>> +{
>> +       char *str = simple_itoa(value);
>> +
>> +       return setenv(varname, str);
>> +}
>
> could be a one liner, but works either way

OK, was trying to separate that out deliberately.

>
>> +int setenv_addr(const char *varname, const void *addr)
>> +{
>> +       char str[17];
>
> char str[sizeof(addr) * 2 + 1];

Yes of course!

>
>> +       sprintf(str, "%x", (uintptr_t)addr);
>
> i wonder if we should use %p and drop the cast
> -mike
>

Is %p supposed to print a 0x before it or not? I saw some discussion
about this. I vote for %p no, and %#p yes.

I will tidy these up for v3.

Regards,
Simon
Mike Frysinger - Oct. 25, 2011, 9:39 p.m.
On Tue, Oct 25, 2011 at 17:35, Simon Glass wrote:
> On Sat, Oct 22, 2011 at 10:29 PM, Mike Frysinger wrote:
>> On Sat, Oct 22, 2011 at 00:51, Simon Glass wrote:
>>> +       sprintf(str, "%x", (uintptr_t)addr);
>>
>> i wonder if we should use %p and drop the cast
>
> Is %p supposed to print a 0x before it or not? I saw some discussion
> about this. I vote for %p no, and %#p yes.

%p does not currently output a leading 0x.  it should (imo), and i'll
prob send a patch to fix that.

but %x doesn't output a leading 0x either :).
-mike
Simon Glass - Oct. 25, 2011, 9:58 p.m.
Hi Mike,

On Tue, Oct 25, 2011 at 2:39 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tue, Oct 25, 2011 at 17:35, Simon Glass wrote:
>> On Sat, Oct 22, 2011 at 10:29 PM, Mike Frysinger wrote:
>>> On Sat, Oct 22, 2011 at 00:51, Simon Glass wrote:
>>>> +       sprintf(str, "%x", (uintptr_t)addr);
>>>
>>> i wonder if we should use %p and drop the cast
>>
>> Is %p supposed to print a 0x before it or not? I saw some discussion
>> about this. I vote for %p no, and %#p yes.
>
> %p does not currently output a leading 0x.  it should (imo), and i'll
> prob send a patch to fix that.
>
> but %x doesn't output a leading 0x either :).
> -mike
>

OK, well to be consistent with glibc we should probably add 0x as you say.

isn't it intended that env variables without a leading 0x in the value
be interpreted as hex still? Iwc do I really want the 0x?

Regards,
Simon
Wolfgang Denk - Oct. 25, 2011, 10:01 p.m.
Dear Simon Glass,

In message <CAPnjgZ1+nTaAjYxq5Nh3i7XTh+RhPC9edVtHF34YNPA2PJcCuw@mail.gmail.com> you wrote:
> 
> isn't it intended that env variables without a leading 0x in the value
> be interpreted as hex still? Iwc do I really want the 0x?

Yes, this is intended.  No, we do NOT want to see the 0x in such
cases.

Best regards,

Wolfgang Denk
Mike Frysinger - Oct. 25, 2011, 10:08 p.m.
On Tue, Oct 25, 2011 at 18:01, Wolfgang Denk wrote:
> Simon Glass wrote:
>> isn't it intended that env variables without a leading 0x in the value
>> be interpreted as hex still? Iwc do I really want the 0x?
>
> Yes, this is intended.  No, we do NOT want to see the 0x in such
> cases.

ok, so we'll want to stick with %x like you already have Simon.  i
disagree, but not enough to fight over it ;).
-mike
Simon Glass - Oct. 25, 2011, 10:41 p.m.
Hi Mike,

On Tue, Oct 25, 2011 at 3:08 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tue, Oct 25, 2011 at 18:01, Wolfgang Denk wrote:
>> Simon Glass wrote:
>>> isn't it intended that env variables without a leading 0x in the value
>>> be interpreted as hex still? Iwc do I really want the 0x?
>>
>> Yes, this is intended.  No, we do NOT want to see the 0x in such
>> cases.
>
> ok, so we'll want to stick with %x like you already have Simon.  i
> disagree, but not enough to fight over it ;).
> -mike
>

OK. But I can use %p if you don't send your patch. How about %#p to
display 0x? It would be incompatible with glibc, but seems more
sensible. Then we could use this in quite a few places in U-Boot I
suspect.

Regards,
Simon
Mike Frysinger - Oct. 25, 2011, 10:49 p.m.
On Tue, Oct 25, 2011 at 18:41, Simon Glass wrote:
> On Tue, Oct 25, 2011 at 3:08 PM, Mike Frysinger wrote:
>> On Tue, Oct 25, 2011 at 18:01, Wolfgang Denk wrote:
>>> Simon Glass wrote:
>>>> isn't it intended that env variables without a leading 0x in the value
>>>> be interpreted as hex still? Iwc do I really want the 0x?
>>>
>>> Yes, this is intended.  No, we do NOT want to see the 0x in such
>>> cases.
>>
>> ok, so we'll want to stick with %x like you already have Simon.  i
>> disagree, but not enough to fight over it ;).
>
> OK. But I can use %p if you don't send your patch. How about %#p to
> display 0x? It would be incompatible with glibc, but seems more
> sensible. Then we could use this in quite a few places in U-Boot I
> suspect.

i think wolfgang's point is that he doesn't want "0x" in env vars.  i
don't think he made any statement about %p in general.

also, %#p works the same under u-boot as under glibc.  it's just that
%#p generates a gcc warning.
-mike
Simon Glass - Oct. 25, 2011, 11:08 p.m.
Hi Mike,

On Tue, Oct 25, 2011 at 3:49 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tue, Oct 25, 2011 at 18:41, Simon Glass wrote:
>> On Tue, Oct 25, 2011 at 3:08 PM, Mike Frysinger wrote:
>>> On Tue, Oct 25, 2011 at 18:01, Wolfgang Denk wrote:
>>>> Simon Glass wrote:
>>>>> isn't it intended that env variables without a leading 0x in the value
>>>>> be interpreted as hex still? Iwc do I really want the 0x?
>>>>
>>>> Yes, this is intended.  No, we do NOT want to see the 0x in such
>>>> cases.
>>>
>>> ok, so we'll want to stick with %x like you already have Simon.  i
>>> disagree, but not enough to fight over it ;).
>>
>> OK. But I can use %p if you don't send your patch. How about %#p to
>> display 0x? It would be incompatible with glibc, but seems more
>> sensible. Then we could use this in quite a few places in U-Boot I
>> suspect.
>
> i think wolfgang's point is that he doesn't want "0x" in env vars.  i
> don't think he made any statement about %p in general.

Yes I got that.
>
> also, %#p works the same under u-boot as under glibc.  it's just that
> %#p generates a gcc warning.
> -mike
>

No I meant that for me glibc displays 0x whether or not I put # in
there...seems wrong to me.

Regards,
Simon
Mike Frysinger - Oct. 25, 2011, 11:27 p.m.
On Tue, Oct 25, 2011 at 19:08, Simon Glass wrote:
> On Tue, Oct 25, 2011 at 3:49 PM, Mike Frysinger wrote:
>> also, %#p works the same under u-boot as under glibc.  it's just that
>> %#p generates a gcc warning.
>
> No I meant that for me glibc displays 0x whether or not I put # in
> there...seems wrong to me.

i don't think so.  it's long done this, as does gcc and the Linux
kernel.  fixing u-boot to operate the same way as the rest of the
ecosystem makes sense to me.
-mike

Patch

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 101bc49..da7028c 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -377,6 +377,35 @@  int setenv(const char *varname, const char *varvalue)
 		return _do_env_set(0, 3, (char * const *)argv);
 }
 
+/**
+ * Set an environment variable to an integer value
+ *
+ * @param varname	Environmet variable to set
+ * @param value		Value to set it to
+ * @return 0 if ok, 1 on error
+ */
+int setenv_ulong(const char *varname, ulong value)
+{
+	char *str = simple_itoa(value);
+
+	return setenv(varname, str);
+}
+
+/**
+ * Set an environment variable to an address in hex
+ *
+ * @param varname	Environmet variable to set
+ * @param addr		Value to set it to
+ * @return 0 if ok, 1 on error
+ */
+int setenv_addr(const char *varname, const void *addr)
+{
+	char str[17];
+
+	sprintf(str, "%x", (uintptr_t)addr);
+	return setenv(varname, str);
+}
+
 int do_env_set(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	if (argc < 2)
diff --git a/include/common.h b/include/common.h
index 55f772d..99c2a37 100644
--- a/include/common.h
+++ b/include/common.h
@@ -294,6 +294,8 @@  int inline setenv    (const char *, const char *);
 #else
 int	setenv	     (const char *, const char *);
 #endif /* CONFIG_PPC */
+int setenv_ulong(const char *varname, ulong value);
+int setenv_addr(const char *varname, const void *addr);
 #ifdef CONFIG_ARM
 # include <asm/mach-types.h>
 # include <asm/setup.h>