[U-Boot] [PATCH v2 01/10] Add getenv_ulong() to read an integer from an environment variable

[U-Boot] [PATCH v2 01/10] Add getenv_ulong() to read an integer from an environment variable

[Simon Glass]

This is not an uncommon operation in U-Boot, so let's put it in a common
function.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Fix commit title from getenv_int() to getenv_ulong()

 common/cmd_nvedit.c |    7 +++++++
 include/common.h    |   12 ++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 101bc49..337ae9a 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -540,6 +540,13 @@ int getenv_f(const char *name, char *buf, unsigned len)
 	return -1;
 }
 
+ulong getenv_ulong(const char *name, int base, ulong default_val)
+{
+	const char *str = getenv(name);
+
+	return str ? simple_strtoul(str, NULL, base) : default_val;
+}
+
 #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
 
 int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
diff --git a/include/common.h b/include/common.h
index eb19a44..c2796e2 100644
--- a/include/common.h
+++ b/include/common.h
@@ -288,6 +288,18 @@ void	env_relocate (void);
 int	envmatch     (uchar *, int);
 char	*getenv	     (const char *);
 int	getenv_f     (const char *name, char *buf, unsigned len);
+
+/**
+ * Decode the value of an environment variable and return it.
+ *
+ * @param name		Name of environemnt variable
+ * @param base		Number base to use (normally 10, or 16 for hex)
+ * @param default_val	Default value to return if the variable is not
+ *			found
+ * @return the decoded value, or default_val if not found
+ */
+ulong getenv_ulong(const char *name, int base, ulong default_val);
+
 int	saveenv	     (void);
 #ifdef CONFIG_PPC		/* ARM version to be fixed! */
 int inline setenv    (const char *, const char *);
-- 
1.7.3.1

[U-Boot] [PATCH v2 01/10] Add getenv_ulong() to read an integer from an environment variable

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111014/f3968bc6/attachment.pgp 

[U-Boot] [PATCH v2 01/10] Add getenv_ulong() to read an integer from an environment variable

[Wolfgang Denk]

Dear Simon Glass,

In message <1318610916-6975-2-git-send-email-sjg@chromium.org> you wrote:
> This is not an uncommon operation in U-Boot, so let's put it in a common
> function.

Please make sure to use this function ONLY in places where getenv()
use is OK.  Do NOT use it where getenv_f() is needed instead.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
People are always a lot more complicated than you  think.  It's  very
important to remember that.             - Terry Pratchett, _Truckers_

[U-Boot] [PATCH v3 01/10] Add getenv_ulong() to read an integer from an environment variable

[Simon Glass]

This is not an uncommon operation in U-Boot, so let's put it in a common
function.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Fix commit title from getenv_int() to getenv_ulong()

Changes in v3:
- Move getenv_ulong() function comment into C file
- Add special code for early environment access

 common/cmd_nvedit.c |   25 +++++++++++++++++++++++++
 include/common.h    |    1 +
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 101bc49..1456b99 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -540,6 +540,31 @@ int getenv_f(const char *name, char *buf, unsigned len)
 	return -1;
 }
 
+/**
+ * Decode the value of an environment variable and return it.
+ *
+ * @param name		Name of environemnt variable
+ * @param base		Number base to use (normally 10, or 16 for hex)
+ * @param default_val	Default value to return if the variable is not
+ *			found
+ * @return the decoded value, or default_val if not found
+ */
+ulong getenv_ulong(const char *name, int base, ulong default_val)
+{
+	char buff[20];
+	const char *str = NULL;
+
+	/*
+	 * Prior to the import of the environment into the hashtable we
+	 * should not call getenv()
+	 */
+	if (gd->flags & GD_FLG_ENV_READY)
+		str = getenv(name);
+	else if (getenv_f(name, buff, sizeof(buff)) > 0)
+		str = buff;
+	return str ? simple_strtoul(str, NULL, base) : default_val;
+}
+
 #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
 
 int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
diff --git a/include/common.h b/include/common.h
index eb19a44..27f5e98 100644
--- a/include/common.h
+++ b/include/common.h
@@ -288,6 +288,7 @@ void	env_relocate (void);
 int	envmatch     (uchar *, int);
 char	*getenv	     (const char *);
 int	getenv_f     (const char *name, char *buf, unsigned len);
+ulong getenv_ulong(const char *name, int base, ulong default_val);
 int	saveenv	     (void);
 #ifdef CONFIG_PPC		/* ARM version to be fixed! */
 int inline setenv    (const char *, const char *);
-- 
1.7.3.1

[U-Boot] [PATCH v3 01/10] Add getenv_ulong() to read an integer from an environment variable

[Wolfgang Denk]

Dear Simon Glass,

In message <1318626284-11161-1-git-send-email-sjg@chromium.org> you wrote:
> This is not an uncommon operation in U-Boot, so let's put it in a common
> function.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
...
> +ulong getenv_ulong(const char *name, int base, ulong default_val)
> +{
> +	char buff[20];
> +	const char *str = NULL;
> +
> +	/*
> +	 * Prior to the import of the environment into the hashtable we
> +	 * should not call getenv()
> +	 */
> +	if (gd->flags & GD_FLG_ENV_READY)
> +		str = getenv(name);
> +	else if (getenv_f(name, buff, sizeof(buff)) > 0)
> +		str = buff;
> +	return str ? simple_strtoul(str, NULL, base) : default_val;
> +}

Sorry, I just changed my mind.

The issue with using getenv() before relocation is that it uses just a
tiny buffer (usually 32 bytes) in the global data structure to store
the result, which is often not sufficient for user provided data (some
boards have hwconfig strings that are _much_ longer than that, and
only the caller knows what to expect).

It's only now that I realize that we are dealing with int / long
values here only, so the length of the expected strings is indeed
limited - actually the buffer you provide here is way smaller that
what getenv() uses.

Can we please switch back to the previous version, but insert a
comment that explains why getenv_f() is not needed in this specific
case?

Thanks, and apologies for causing additional efforts.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Status quo. Latin for "the mess we're in."

[U-Boot] [PATCH v4 01/10] Add getenv_ulong() to read an integer from an environment variable

[Simon Glass]

This is not an uncommon operation in U-Boot, so let's put it in a common
function.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Fix commit title from getenv_int() to getenv_ulong()

Changes in v3:
- Move getenv_ulong() function comment into C file
- Add special code for early environment access

Changes in v4:
- Change getenv_ulong() implementation back to version 2
- Add a comment as to why this is ok

 common/cmd_nvedit.c |   20 ++++++++++++++++++++
 include/common.h    |    1 +
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 101bc49..fa99c44 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -540,6 +540,26 @@ int getenv_f(const char *name, char *buf, unsigned len)
 	return -1;
 }
 
+/**
+ * Decode the integer value of an environment variable and return it.
+ *
+ * @param name		Name of environemnt variable
+ * @param base		Number base to use (normally 10, or 16 for hex)
+ * @param default_val	Default value to return if the variable is not
+ *			found
+ * @return the decoded value, or default_val if not found
+ */
+ulong getenv_ulong(const char *name, int base, ulong default_val)
+{
+	/*
+	 * We can use getenv() here, even before relocation, since the
+	 * environment variable value is an integer and thus short.
+	 */
+	const char *str = getenv(name);
+
+	return str ? simple_strtoul(str, NULL, base) : default_val;
+}
+
 #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
 
 int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
diff --git a/include/common.h b/include/common.h
index eb19a44..27f5e98 100644
--- a/include/common.h
+++ b/include/common.h
@@ -288,6 +288,7 @@ void	env_relocate (void);
 int	envmatch     (uchar *, int);
 char	*getenv	     (const char *);
 int	getenv_f     (const char *name, char *buf, unsigned len);
+ulong getenv_ulong(const char *name, int base, ulong default_val);
 int	saveenv	     (void);
 #ifdef CONFIG_PPC		/* ARM version to be fixed! */
 int inline setenv    (const char *, const char *);
-- 
1.7.3.1

[U-Boot] [PATCH v3 01/10] Add getenv_ulong() to read an integer from an environment variable

[Simon Glass]

Hi Wolfgang,

On Fri, Oct 14, 2011 at 2:42 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1318626284-11161-1-git-send-email-sjg@chromium.org> you wrote:
>> This is not an uncommon operation in U-Boot, so let's put it in a common
>> function.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
> ...
>> +ulong getenv_ulong(const char *name, int base, ulong default_val)
>> +{
>> + ? ? char buff[20];
>> + ? ? const char *str = NULL;
>> +
>> + ? ? /*
>> + ? ? ?* Prior to the import of the environment into the hashtable we
>> + ? ? ?* should not call getenv()
>> + ? ? ?*/
>> + ? ? if (gd->flags & GD_FLG_ENV_READY)
>> + ? ? ? ? ? ? str = getenv(name);
>> + ? ? else if (getenv_f(name, buff, sizeof(buff)) > 0)
>> + ? ? ? ? ? ? str = buff;
>> + ? ? return str ? simple_strtoul(str, NULL, base) : default_val;
>> +}
>
> Sorry, I just changed my mind.
>
> The issue with using getenv() before relocation is that it uses just a
> tiny buffer (usually 32 bytes) in the global data structure to store
> the result, which is often not sufficient for user provided data (some
> boards have hwconfig strings that are _much_ longer than that, and
> only the caller knows what to expect).
>
> It's only now that I realize that we are dealing with int / long
> values here only, so the length of the expected strings is indeed
> limited - actually the buffer you provide here is way smaller that
> what getenv() uses.
>
> Can we please switch back to the previous version, but insert a
> comment that explains why getenv_f() is not needed in this specific
> case?

OK I have done that and sent version 4 of that patch.

>
> Thanks, and apologies for causing additional efforts.

No problem.

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Status quo. Latin for "the mess we're in."
>

[U-Boot] [PATCH v4 01/10] Add getenv_ulong() to read an integer from an environment variable

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111015/e135654c/attachment.pgp 

[U-Boot] [PATCH v4 01/10] Add getenv_ulong() to read an integer from an environment variable

[Wolfgang Denk]

Dear Simon Glass,

In message <1318634718-10548-1-git-send-email-sjg@chromium.org> you wrote:
> This is not an uncommon operation in U-Boot, so let's put it in a common
> function.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v2:
> - Fix commit title from getenv_int() to getenv_ulong()
> 
> Changes in v3:
> - Move getenv_ulong() function comment into C file
> - Add special code for early environment access
> 
> Changes in v4:
> - Change getenv_ulong() implementation back to version 2
> - Add a comment as to why this is ok
> 
>  common/cmd_nvedit.c |   20 ++++++++++++++++++++
>  include/common.h    |    1 +
>  2 files changed, 21 insertions(+), 0 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It must be remembered that there is nothing more difficult  to  plan,
more  doubtful  of  success,  nor  more dangerous to manage, than the
creation of a new system. For the initiator has the enmity of all who
would profit by the preservation of the old institutions  and  merely
lukewarm defenders in those who would gain by the new ones.
- Machiavelli