* [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor
@ 2021-10-13 15:45 Marek Behún
2021-10-13 15:45 ` [PATCH v2 01/13] env: Fix documentation for env_get_f() Marek Behún
` (12 more replies)
0 siblings, 13 replies; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Hi Simon, Tom,
env_get_char() is a relic from the past when env was read char-by-char
from underlying device. Currently it only accesses in-memory arrays.
We can remove it and access the arrays directly. This simplifies the old
code of env_get_f().
Changes since v1:
- use memcpy() instead of strncpy() when copying value to buffer
- fixed a bug in patch adding check to terminating NULL-byte
- added patch fixing documentation for env_get_f()
- added patch changing behaviour of return value of env_get_f()
- some other cosmetic changes
Marek
Marek Behún (13):
env: Fix documentation for env_get_f()
env: Drop env_get_char_spec() and old, unused .get_char()
implementations
examples: api: glue: Remove comment that does not apply anymore
env: Change env_match() to static and remove from header
env: Don't match empty variable name in env_match()
env: Check for terminating null-byte in env_match()
env: Inline env_get_char() into it's only user
env: Early return from env_get_f() on NULL name
env: Use string pointer instead of indexes in env_get_f()
env: Use better name for variable in env_get_f()
env: Make return value of env_get_f() behave like sprintf() on success
env: Use memcpy() instead of ad-hoc code to copy variable value
env: Move non-cmd specific env functions to env/common.c
cmd/nvedit.c | 188 -------------------------------------------
env/common.c | 190 ++++++++++++++++++++++++++++++++++++++++++++
env/eeprom.c | 18 -----
env/env.c | 13 ---
env/nowhere.c | 5 +-
env/nvram.c | 14 ----
examples/api/glue.c | 5 --
include/env.h | 24 +-----
8 files changed, 194 insertions(+), 263 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 01/13] env: Fix documentation for env_get_f()
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 02/13] env: Drop env_get_char_spec() and old, unused .get_char() implementations Marek Behún
` (11 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
This function actually returns:
- the number of bytes written into @buf excluding the terminating
NULL-byte, if there was enough space in @buf
- the number of bytes written into @buf including the terminating
NULL-byte, if there wasn't enough space in @buf
- -1 if the variable is not found
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
include/env.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/env.h b/include/env.h
index d5e2bcb530..b1a4003681 100644
--- a/include/env.h
+++ b/include/env.h
@@ -131,7 +131,10 @@ char *from_env(const char *envvar);
* support reading the value (slowly) and some will not.
*
* @varname: Variable to look up
- * @return value of variable, or NULL if not found
+ * @return number of bytes written into @buf, excluding the terminating
+ * NULL-byte if there was enough space in @buf, and including the
+ * terminating NULL-byte if there wasn't enough space, or -1 if the
+ * variable is not found
*/
int env_get_f(const char *name, char *buf, unsigned int len);
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 02/13] env: Drop env_get_char_spec() and old, unused .get_char() implementations
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
2021-10-13 15:45 ` [PATCH v2 01/13] env: Fix documentation for env_get_f() Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 03/13] examples: api: glue: Remove comment that does not apply anymore Marek Behún
` (10 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Commit b2cdef4861be ("env: restore old env_get_char() behaviour")
dropped the .get_char() method from struct env_driver, but left the two
existing implementations (eeprom and nvram) in case someone would use
them by overwriting weak function env_get_char_spec().
Since this was never done in the 3.5 years, let's drop these methods and
simplify the code.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
env/eeprom.c | 18 ------------------
env/env.c | 7 +------
env/nvram.c | 14 --------------
3 files changed, 1 insertion(+), 38 deletions(-)
diff --git a/env/eeprom.c b/env/eeprom.c
index 253bdf1428..f8556a4721 100644
--- a/env/eeprom.c
+++ b/env/eeprom.c
@@ -64,24 +64,6 @@ static int eeprom_bus_write(unsigned dev_addr, unsigned offset,
return rcode;
}
-/** Call this function from overridden env_get_char_spec() if you need
- * this functionality.
- */
-int env_eeprom_get_char(int index)
-{
- uchar c;
- unsigned int off = CONFIG_ENV_OFFSET;
-
-#ifdef CONFIG_ENV_OFFSET_REDUND
- if (gd->env_valid == ENV_REDUND)
- off = CONFIG_ENV_OFFSET_REDUND;
-#endif
- eeprom_bus_read(CONFIG_SYS_I2C_EEPROM_ADDR,
- off + index + offsetof(env_t, data), &c, 1);
-
- return c;
-}
-
static int env_eeprom_load(void)
{
char buf_env[CONFIG_ENV_SIZE];
diff --git a/env/env.c b/env/env.c
index e534008006..91d220c3dd 100644
--- a/env/env.c
+++ b/env/env.c
@@ -166,17 +166,12 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
return drv;
}
-__weak int env_get_char_spec(int index)
-{
- return *(uchar *)(gd->env_addr + index);
-}
-
int env_get_char(int index)
{
if (gd->env_valid == ENV_INVALID)
return default_environment[index];
else
- return env_get_char_spec(index);
+ return *(uchar *)(gd->env_addr + index);
}
int env_load(void)
diff --git a/env/nvram.c b/env/nvram.c
index f4126858b5..261b31edfb 100644
--- a/env/nvram.c
+++ b/env/nvram.c
@@ -42,20 +42,6 @@ extern void nvram_write(long dest, const void *src, size_t count);
static env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR;
#endif
-#ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE
-/** Call this function from overridden env_get_char_spec() if you need
- * this functionality.
- */
-int env_nvram_get_char(int index)
-{
- uchar c;
-
- nvram_read(&c, CONFIG_ENV_ADDR + index, 1);
-
- return c;
-}
-#endif
-
static int env_nvram_load(void)
{
char buf[CONFIG_ENV_SIZE];
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 03/13] examples: api: glue: Remove comment that does not apply anymore
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
2021-10-13 15:45 ` [PATCH v2 01/13] env: Fix documentation for env_get_f() Marek Behún
2021-10-13 15:45 ` [PATCH v2 02/13] env: Drop env_get_char_spec() and old, unused .get_char() implementations Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 04/13] env: Change env_match() to static and remove from header Marek Behún
` (9 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
This comment is not true since commit 6215bd4c1fd6 ("api: Use hashtable
function for API_env_enum").
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
examples/api/glue.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/examples/api/glue.c b/examples/api/glue.c
index 91d13157a1..075d307ae2 100644
--- a/examples/api/glue.c
+++ b/examples/api/glue.c
@@ -365,11 +365,6 @@ const char * ub_env_enum(const char *last)
env = NULL;
- /*
- * It's OK to pass only the name piece as last (and not the whole
- * 'name=val' string), since the API_ENUM_ENV call uses env_match()
- * internally, which handles such case
- */
if (!syscall(API_ENV_ENUM, NULL, last, &env))
return NULL;
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 04/13] env: Change env_match() to static and remove from header
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
` (2 preceding siblings ...)
2021-10-13 15:45 ` [PATCH v2 03/13] examples: api: glue: Remove comment that does not apply anymore Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 05/13] env: Don't match empty variable name in env_match() Marek Behún
` (8 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
This function was used by other parts of U-Boot in the past when
environment was read from underlying device one character at a time.
This is not the case anymore.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 30 +++++++++++++++---------------
include/env.h | 11 -----------
2 files changed, 15 insertions(+), 26 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index ddc715b4f9..742e0924af 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -706,6 +706,21 @@ char *from_env(const char *envvar)
return ret;
}
+static int env_match(uchar *s1, int i2)
+{
+ if (s1 == NULL)
+ return -1;
+
+ while (*s1 == env_get_char(i2++))
+ if (*s1++ == '=')
+ return i2;
+
+ if (*s1 == '\0' && env_get_char(i2-1) == '=')
+ return i2;
+
+ return -1;
+}
+
/*
* Look up variable from environment for restricted C runtime env.
*/
@@ -816,21 +831,6 @@ static int do_env_select(struct cmd_tbl *cmdtp, int flag, int argc,
#endif /* CONFIG_SPL_BUILD */
-int env_match(uchar *s1, int i2)
-{
- if (s1 == NULL)
- return -1;
-
- while (*s1 == env_get_char(i2++))
- if (*s1++ == '=')
- return i2;
-
- if (*s1 == '\0' && env_get_char(i2-1) == '=')
- return i2;
-
- return -1;
-}
-
#ifndef CONFIG_SPL_BUILD
static int do_env_default(struct cmd_tbl *cmdtp, int flag,
int argc, char *const argv[])
diff --git a/include/env.h b/include/env.h
index b1a4003681..a9b2a4c8b2 100644
--- a/include/env.h
+++ b/include/env.h
@@ -90,17 +90,6 @@ int env_init(void);
*/
void env_relocate(void);
-/**
- * env_match() - Match a name / name=value pair
- *
- * This is used prior to relocation for finding envrionment variables
- *
- * @name: A simple 'name', or a 'name=value' pair.
- * @index: The environment index for a 'name2=value2' pair.
- * @return index for the value if the names match, else -1.
- */
-int env_match(unsigned char *name, int index);
-
/**
* env_get() - Look up the value of an environment variable
*
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 05/13] env: Don't match empty variable name in env_match()
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
` (3 preceding siblings ...)
2021-10-13 15:45 ` [PATCH v2 04/13] env: Change env_match() to static and remove from header Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 06/13] env: Check for terminating null-byte " Marek Behún
` (7 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Do we really allow zero-length variable name? I guess not.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 742e0924af..e2e8a38b5d 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -708,7 +708,7 @@ char *from_env(const char *envvar)
static int env_match(uchar *s1, int i2)
{
- if (s1 == NULL)
+ if (s1 == NULL || *s1 == '\0')
return -1;
while (*s1 == env_get_char(i2++))
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 06/13] env: Check for terminating null-byte in env_match()
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
` (4 preceding siblings ...)
2021-10-13 15:45 ` [PATCH v2 05/13] env: Don't match empty variable name in env_match() Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 07/13] env: Inline env_get_char() into it's only user Marek Behún
` (6 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
There is a possible overflow in env_match(): if environment contains
a terminating null-byte before '=' character (i.e. environment is
broken), the env_match() function can access data after the terminating
null-byte from parameter pointer.
Example: if env_get_char() returns characters from string array
"abc\0def\0" and env_match("abc", 0) is called, the function will access
at least one byte after the end of the "abc" literal.
Fix this by checking for terminating null-byte in env_match().
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
Change since v1:
- check for '\0' only after incrementing i2
---
cmd/nvedit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index e2e8a38b5d..a22445132b 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -711,7 +711,7 @@ static int env_match(uchar *s1, int i2)
if (s1 == NULL || *s1 == '\0')
return -1;
- while (*s1 == env_get_char(i2++))
+ while (*s1 == env_get_char(i2++) && *s1 != '\0')
if (*s1++ == '=')
return i2;
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 07/13] env: Inline env_get_char() into it's only user
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
` (5 preceding siblings ...)
2021-10-13 15:45 ` [PATCH v2 06/13] env: Check for terminating null-byte " Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 08/13] env: Early return from env_get_f() on NULL name Marek Behún
` (5 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
This function is a relic from the past when environment was read from
underlying device one character at a time.
It is used only in the case when getting an environemnt variable prior
relocation, and the function is simple enough to be inlined there.
Since env_get_char() is being changed to simple access to an array, we
can drop the failing cases and simplify the code (this could have been
done before, since env_get_char() did not fail even before).
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 28 ++++++++++++++--------------
env/env.c | 8 --------
env/nowhere.c | 5 ++---
include/env.h | 10 ----------
4 files changed, 16 insertions(+), 35 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index a22445132b..9f0caceadf 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -706,16 +706,16 @@ char *from_env(const char *envvar)
return ret;
}
-static int env_match(uchar *s1, int i2)
+static int env_match(const char *env, const char *s1, int i2)
{
if (s1 == NULL || *s1 == '\0')
return -1;
- while (*s1 == env_get_char(i2++) && *s1 != '\0')
+ while (*s1 == env[i2++] && *s1 != '\0')
if (*s1++ == '=')
return i2;
- if (*s1 == '\0' && env_get_char(i2-1) == '=')
+ if (*s1 == '\0' && env[i2-1] == '=')
return i2;
return -1;
@@ -726,28 +726,28 @@ static int env_match(uchar *s1, int i2)
*/
int env_get_f(const char *name, char *buf, unsigned len)
{
- int i, nxt, c;
+ const char *env;
+ int i, nxt;
- for (i = 0; env_get_char(i) != '\0'; i = nxt + 1) {
+ if (gd->env_valid == ENV_INVALID)
+ env = (const char *)default_environment;
+ else
+ env = (const char *)gd->env_addr;
+
+ for (i = 0; env[i] != '\0'; i = nxt + 1) {
int val, n;
- for (nxt = i; (c = env_get_char(nxt)) != '\0'; ++nxt) {
- if (c < 0)
- return c;
+ for (nxt = i; env[nxt] != '\0'; ++nxt)
if (nxt >= CONFIG_ENV_SIZE)
return -1;
- }
- val = env_match((uchar *)name, i);
+ val = env_match(env, name, i);
if (val < 0)
continue;
/* found; copy out */
for (n = 0; n < len; ++n, ++buf) {
- c = env_get_char(val++);
- if (c < 0)
- return c;
- *buf = c;
+ *buf = env[val++];
if (*buf == '\0')
return n;
}
diff --git a/env/env.c b/env/env.c
index 91d220c3dd..e4dfb92e15 100644
--- a/env/env.c
+++ b/env/env.c
@@ -166,14 +166,6 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
return drv;
}
-int env_get_char(int index)
-{
- if (gd->env_valid == ENV_INVALID)
- return default_environment[index];
- else
- return *(uchar *)(gd->env_addr + index);
-}
-
int env_load(void)
{
struct env_driver *drv;
diff --git a/env/nowhere.c b/env/nowhere.c
index 41557f5ce4..1fcf503453 100644
--- a/env/nowhere.c
+++ b/env/nowhere.c
@@ -31,9 +31,8 @@ static int env_nowhere_init(void)
static int env_nowhere_load(void)
{
/*
- * for SPL, set env_valid = ENV_INVALID is enough as env_get_char()
- * return the default env if env_get is used
- * and SPL don't used env_import to reduce its size
+ * For SPL, setting env_valid = ENV_INVALID is enough, as env_get()
+ * searches default_environment array in that case.
* For U-Boot proper, import the default environment to allow reload.
*/
if (!IS_ENABLED(CONFIG_SPL_BUILD))
diff --git a/include/env.h b/include/env.h
index a9b2a4c8b2..220ab979d9 100644
--- a/include/env.h
+++ b/include/env.h
@@ -351,16 +351,6 @@ char *env_get_default(const char *name);
/* [re]set to the default environment */
void env_set_default(const char *s, int flags);
-/**
- * env_get_char() - Get a character from the early environment
- *
- * This reads from the pre-relocation environment
- *
- * @index: Index of character to read (0 = first)
- * @return character read, or -ve on error
- */
-int env_get_char(int index);
-
/**
* env_reloc() - Relocate the 'env' sub-commands
*
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 08/13] env: Early return from env_get_f() on NULL name
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
` (6 preceding siblings ...)
2021-10-13 15:45 ` [PATCH v2 07/13] env: Inline env_get_char() into it's only user Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 09/13] env: Use string pointer instead of indexes in env_get_f() Marek Behún
` (4 subsequent siblings)
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Test non-NULL name immediately, not in env_match().
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 9f0caceadf..7c99a693ea 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -708,13 +708,11 @@ char *from_env(const char *envvar)
static int env_match(const char *env, const char *s1, int i2)
{
- if (s1 == NULL || *s1 == '\0')
- return -1;
-
while (*s1 == env[i2++] && *s1 != '\0')
if (*s1++ == '=')
return i2;
+ /* We can look at env[i2-1] because i2 was incremented at least once. */
if (*s1 == '\0' && env[i2-1] == '=')
return i2;
@@ -729,6 +727,9 @@ int env_get_f(const char *name, char *buf, unsigned len)
const char *env;
int i, nxt;
+ if (name == NULL || *name == '\0')
+ return -1;
+
if (gd->env_valid == ENV_INVALID)
env = (const char *)default_environment;
else
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 09/13] env: Use string pointer instead of indexes in env_get_f()
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
` (7 preceding siblings ...)
2021-10-13 15:45 ` [PATCH v2 08/13] env: Early return from env_get_f() on NULL name Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-15 0:40 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 10/13] env: Use better name for variable " Marek Behún
` (3 subsequent siblings)
12 siblings, 2 replies; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Since we no longer use env_get_char() to access n-th character of
linearized environment data, but rather access the arrays themselves, we
can convert the iteration to use string pointers instead of position
indexes.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 7c99a693ea..6eabd51209 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -706,17 +706,17 @@ char *from_env(const char *envvar)
return ret;
}
-static int env_match(const char *env, const char *s1, int i2)
+static const char *matching_name_get_value(const char *p, const char *name)
{
- while (*s1 == env[i2++] && *s1 != '\0')
- if (*s1++ == '=')
- return i2;
+ while (*name == *p++ && *name != '\0')
+ if (*name++ == '=')
+ return p;
- /* We can look at env[i2-1] because i2 was incremented at least once. */
- if (*s1 == '\0' && env[i2-1] == '=')
- return i2;
+ /* We can look at p[-1] because p was incremented at least once. */
+ if (*name == '\0' && p[-1] == '=')
+ return p;
- return -1;
+ return NULL;
}
/*
@@ -724,8 +724,7 @@ static int env_match(const char *env, const char *s1, int i2)
*/
int env_get_f(const char *name, char *buf, unsigned len)
{
- const char *env;
- int i, nxt;
+ const char *env, *p, *nxt;
if (name == NULL || *name == '\0')
return -1;
@@ -735,20 +734,21 @@ int env_get_f(const char *name, char *buf, unsigned len)
else
env = (const char *)gd->env_addr;
- for (i = 0; env[i] != '\0'; i = nxt + 1) {
- int val, n;
+ for (p = env; *p != '\0'; p = nxt + 1) {
+ const char *value;
+ int n;
- for (nxt = i; env[nxt] != '\0'; ++nxt)
- if (nxt >= CONFIG_ENV_SIZE)
+ for (nxt = p; *nxt != '\0'; ++nxt)
+ if (nxt - env >= CONFIG_ENV_SIZE)
return -1;
- val = env_match(env, name, i);
- if (val < 0)
+ value = matching_name_get_value(p, name);
+ if (value == NULL)
continue;
/* found; copy out */
for (n = 0; n < len; ++n, ++buf) {
- *buf = env[val++];
+ *buf = *value++;
if (*buf == '\0')
return n;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 10/13] env: Use better name for variable in env_get_f()
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
` (8 preceding siblings ...)
2021-10-13 15:45 ` [PATCH v2 09/13] env: Use string pointer instead of indexes in env_get_f() Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-15 0:40 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 11/13] env: Make return value of env_get_f() behave like sprintf() on success Marek Behún
` (2 subsequent siblings)
12 siblings, 2 replies; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
The `nxt` variable actually points to the terminating null-byte of the
current env var, and the next env var is at `nxt + 1`, not `nxt`. So a
better name for this variable is `end`.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 6eabd51209..08288fad10 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -724,7 +724,7 @@ static const char *matching_name_get_value(const char *p, const char *name)
*/
int env_get_f(const char *name, char *buf, unsigned len)
{
- const char *env, *p, *nxt;
+ const char *env, *p, *end;
if (name == NULL || *name == '\0')
return -1;
@@ -734,12 +734,12 @@ int env_get_f(const char *name, char *buf, unsigned len)
else
env = (const char *)gd->env_addr;
- for (p = env; *p != '\0'; p = nxt + 1) {
+ for (p = env; *p != '\0'; p = end + 1) {
const char *value;
int n;
- for (nxt = p; *nxt != '\0'; ++nxt)
- if (nxt - env >= CONFIG_ENV_SIZE)
+ for (end = p; *end != '\0'; ++end)
+ if (end - env >= CONFIG_ENV_SIZE)
return -1;
value = matching_name_get_value(p, name);
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 11/13] env: Make return value of env_get_f() behave like sprintf() on success
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
` (9 preceding siblings ...)
2021-10-13 15:45 ` [PATCH v2 10/13] env: Use better name for variable " Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 12/13] env: Use memcpy() instead of ad-hoc code to copy variable value Marek Behún
2021-10-13 15:45 ` [PATCH v2 13/13] env: Move non-cmd specific env functions to env/common.c Marek Behún
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Currently the env_get_f() function's return value behaves weirdly: it
returns the number of bytes written into `buf`, but whether this is
excluding the terminating NULL-byte or including it depends on whether
there was enough space in `buf`.
Change the function to always return the actual length of the value of
the environment variable (excluding the terminating NULL-byte) on
success. This makes it behave like sprintf().
All users of this function in U-Boot are compatible with this change.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 8 +++++---
include/env.h | 6 ++----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 08288fad10..8989c85d20 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -736,7 +736,7 @@ int env_get_f(const char *name, char *buf, unsigned len)
for (p = env; *p != '\0'; p = end + 1) {
const char *value;
- int n;
+ int n, res;
for (end = p; *end != '\0'; ++end)
if (end - env >= CONFIG_ENV_SIZE)
@@ -746,11 +746,13 @@ int env_get_f(const char *name, char *buf, unsigned len)
if (value == NULL)
continue;
+ res = end - value;
+
/* found; copy out */
for (n = 0; n < len; ++n, ++buf) {
*buf = *value++;
if (*buf == '\0')
- return n;
+ return res;
}
if (n)
@@ -759,7 +761,7 @@ int env_get_f(const char *name, char *buf, unsigned len)
printf("env_buf [%u bytes] too small for value of \"%s\"\n",
len, name);
- return n;
+ return res;
}
return -1;
diff --git a/include/env.h b/include/env.h
index 220ab979d9..ee5e30d036 100644
--- a/include/env.h
+++ b/include/env.h
@@ -120,10 +120,8 @@ char *from_env(const char *envvar);
* support reading the value (slowly) and some will not.
*
* @varname: Variable to look up
- * @return number of bytes written into @buf, excluding the terminating
- * NULL-byte if there was enough space in @buf, and including the
- * terminating NULL-byte if there wasn't enough space, or -1 if the
- * variable is not found
+ * @return actual length of the variable value excluding the terminating
+ * NULL-byte, or -1 if the variable is not found
*/
int env_get_f(const char *name, char *buf, unsigned int len);
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 12/13] env: Use memcpy() instead of ad-hoc code to copy variable value
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
` (10 preceding siblings ...)
2021-10-13 15:45 ` [PATCH v2 11/13] env: Make return value of env_get_f() behave like sprintf() on success Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 13/13] env: Move non-cmd specific env functions to env/common.c Marek Behún
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Copy the value of the found variable into given buffer with memcpy()
instead of ad-hoc code.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 8989c85d20..7f094b3cd7 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -736,7 +736,7 @@ int env_get_f(const char *name, char *buf, unsigned len)
for (p = env; *p != '\0'; p = end + 1) {
const char *value;
- int n, res;
+ unsigned res;
for (end = p; *end != '\0'; ++end)
if (end - env >= CONFIG_ENV_SIZE)
@@ -747,20 +747,14 @@ int env_get_f(const char *name, char *buf, unsigned len)
continue;
res = end - value;
+ memcpy(buf, value, min(len, res + 1));
- /* found; copy out */
- for (n = 0; n < len; ++n, ++buf) {
- *buf = *value++;
- if (*buf == '\0')
- return res;
+ if (len <= res) {
+ buf[len - 1] = '\0';
+ printf("env_buf [%u bytes] too small for value of \"%s\"\n",
+ len, name);
}
- if (n)
- *--buf = '\0';
-
- printf("env_buf [%u bytes] too small for value of \"%s\"\n",
- len, name);
-
return res;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 13/13] env: Move non-cmd specific env functions to env/common.c
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
` (11 preceding siblings ...)
2021-10-13 15:45 ` [PATCH v2 12/13] env: Use memcpy() instead of ad-hoc code to copy variable value Marek Behún
@ 2021-10-13 15:45 ` Marek Behún
2021-10-14 15:11 ` Simon Glass
12 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-13 15:45 UTC (permalink / raw)
To: Simon Glass, Tom Rini; +Cc: U-Boot Mailing List, Marek Behún
From: Marek Behún <marek.behun@nic.cz>
Move the following functions from cmd/nvedit.c to env/common.c:
env_set_ulong()
env_set_hex()
env_get_hex()
eth_env_get_enetaddr()
eth_env_set_enetaddr()
env_get()
from_env()
env_get_f()
env_get_ulong()
since these functions are not specific for U-Boot's CLI.
We leave env_set() in cmd/nvedit.c, since it calls _do_env_set(), which
is a static function in that file.
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
cmd/nvedit.c | 185 -------------------------------------------------
env/common.c | 190 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 190 insertions(+), 185 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 7f094b3cd7..3bb6e764c0 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -30,7 +30,6 @@
#include <env.h>
#include <env_internal.h>
#include <log.h>
-#include <net.h>
#include <search.h>
#include <errno.h>
#include <malloc.h>
@@ -38,7 +37,6 @@
#include <asm/global_data.h>
#include <linux/bitops.h>
#include <u-boot/crc.h>
-#include <watchdog.h>
#include <linux/stddef.h>
#include <asm/byteorder.h>
#include <asm/io.h>
@@ -320,69 +318,6 @@ int env_set(const char *varname, const char *varvalue)
return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC);
}
-/**
- * Set an environment variable to an integer value
- *
- * @param varname Environment variable to set
- * @param value Value to set it to
- * @return 0 if ok, 1 on error
- */
-int env_set_ulong(const char *varname, ulong value)
-{
- /* TODO: this should be unsigned */
- char *str = simple_itoa(value);
-
- return env_set(varname, str);
-}
-
-/**
- * Set an environment variable to an value in hex
- *
- * @param varname Environment variable to set
- * @param value Value to set it to
- * @return 0 if ok, 1 on error
- */
-int env_set_hex(const char *varname, ulong value)
-{
- char str[17];
-
- sprintf(str, "%lx", value);
- return env_set(varname, str);
-}
-
-ulong env_get_hex(const char *varname, ulong default_val)
-{
- const char *s;
- ulong value;
- char *endp;
-
- s = env_get(varname);
- if (s)
- value = hextoul(s, &endp);
- if (!s || endp == s)
- return default_val;
-
- return value;
-}
-
-int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr)
-{
- string_to_enetaddr(env_get(name), enetaddr);
- return is_valid_ethaddr(enetaddr);
-}
-
-int eth_env_set_enetaddr(const char *name, const uint8_t *enetaddr)
-{
- char buf[ARP_HLEN_ASCII + 1];
-
- if (eth_env_get_enetaddr(name, (uint8_t *)buf))
- return -EEXIST;
-
- sprintf(buf, "%pM", enetaddr);
-
- return env_set(name, buf);
-}
-
#ifndef CONFIG_SPL_BUILD
static int do_env_set(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
@@ -661,127 +596,7 @@ static int do_env_edit(struct cmd_tbl *cmdtp, int flag, int argc,
}
}
#endif /* CONFIG_CMD_EDITENV */
-#endif /* CONFIG_SPL_BUILD */
-
-/*
- * Look up variable from environment,
- * return address of storage for that variable,
- * or NULL if not found
- */
-char *env_get(const char *name)
-{
- if (gd->flags & GD_FLG_ENV_READY) { /* after import into hashtable */
- struct env_entry e, *ep;
-
- WATCHDOG_RESET();
-
- e.key = name;
- e.data = NULL;
- hsearch_r(e, ENV_FIND, &ep, &env_htab, 0);
-
- return ep ? ep->data : NULL;
- }
-
- /* restricted capabilities before import */
- if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) > 0)
- return (char *)(gd->env_buf);
- return NULL;
-}
-
-/*
- * Like env_get, but prints an error if envvar isn't defined in the
- * environment. It always returns what env_get does, so it can be used in
- * place of env_get without changing error handling otherwise.
- */
-char *from_env(const char *envvar)
-{
- char *ret;
-
- ret = env_get(envvar);
-
- if (!ret)
- printf("missing environment variable: %s\n", envvar);
-
- return ret;
-}
-
-static const char *matching_name_get_value(const char *p, const char *name)
-{
- while (*name == *p++ && *name != '\0')
- if (*name++ == '=')
- return p;
-
- /* We can look at p[-1] because p was incremented at least once. */
- if (*name == '\0' && p[-1] == '=')
- return p;
-
- return NULL;
-}
-
-/*
- * Look up variable from environment for restricted C runtime env.
- */
-int env_get_f(const char *name, char *buf, unsigned len)
-{
- const char *env, *p, *end;
-
- if (name == NULL || *name == '\0')
- return -1;
-
- if (gd->env_valid == ENV_INVALID)
- env = (const char *)default_environment;
- else
- env = (const char *)gd->env_addr;
-
- for (p = env; *p != '\0'; p = end + 1) {
- const char *value;
- unsigned res;
-
- for (end = p; *end != '\0'; ++end)
- if (end - env >= CONFIG_ENV_SIZE)
- return -1;
-
- value = matching_name_get_value(p, name);
- if (value == NULL)
- continue;
-
- res = end - value;
- memcpy(buf, value, min(len, res + 1));
-
- if (len <= res) {
- buf[len - 1] = '\0';
- printf("env_buf [%u bytes] too small for value of \"%s\"\n",
- len, name);
- }
-
- return res;
- }
-
- return -1;
-}
-
-/**
- * Decode the integer value of an environment variable and return it.
- *
- * @param name Name of environment 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 env_get_ulong(const char *name, int base, ulong default_val)
-{
- /*
- * We can use env_get() here, even before relocation, since the
- * environment variable value is an integer and thus short.
- */
- const char *str = env_get(name);
-
- return str ? simple_strtoul(str, NULL, base) : default_val;
-}
-
-#ifndef CONFIG_SPL_BUILD
#if defined(CONFIG_CMD_SAVEENV) && defined(ENV_IS_IN_DEVICE)
static int do_env_save(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
diff --git a/env/common.c b/env/common.c
index 81e9e0b2aa..df3daa7f4e 100644
--- a/env/common.c
+++ b/env/common.c
@@ -21,6 +21,8 @@
#include <malloc.h>
#include <u-boot/crc.h>
#include <dm/ofnode.h>
+#include <net.h>
+#include <watchdog.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -33,6 +35,194 @@ struct hsearch_data env_htab = {
.change_ok = env_flags_validate,
};
+/*
+ * This env_set() function is defined in cmd/nvedit.c, since it calls
+ * _do_env_set(), whis is a static function in that file.
+ *
+ * int env_set(const char *varname, const char *varvalue);
+ */
+
+/**
+ * Set an environment variable to an integer value
+ *
+ * @param varname Environment variable to set
+ * @param value Value to set it to
+ * @return 0 if ok, 1 on error
+ */
+int env_set_ulong(const char *varname, ulong value)
+{
+ /* TODO: this should be unsigned */
+ char *str = simple_itoa(value);
+
+ return env_set(varname, str);
+}
+
+/**
+ * Set an environment variable to an value in hex
+ *
+ * @param varname Environment variable to set
+ * @param value Value to set it to
+ * @return 0 if ok, 1 on error
+ */
+int env_set_hex(const char *varname, ulong value)
+{
+ char str[17];
+
+ sprintf(str, "%lx", value);
+ return env_set(varname, str);
+}
+
+ulong env_get_hex(const char *varname, ulong default_val)
+{
+ const char *s;
+ ulong value;
+ char *endp;
+
+ s = env_get(varname);
+ if (s)
+ value = hextoul(s, &endp);
+ if (!s || endp == s)
+ return default_val;
+
+ return value;
+}
+
+int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr)
+{
+ string_to_enetaddr(env_get(name), enetaddr);
+ return is_valid_ethaddr(enetaddr);
+}
+
+int eth_env_set_enetaddr(const char *name, const uint8_t *enetaddr)
+{
+ char buf[ARP_HLEN_ASCII + 1];
+
+ if (eth_env_get_enetaddr(name, (uint8_t *)buf))
+ return -EEXIST;
+
+ sprintf(buf, "%pM", enetaddr);
+
+ return env_set(name, buf);
+}
+
+/*
+ * Look up variable from environment,
+ * return address of storage for that variable,
+ * or NULL if not found
+ */
+char *env_get(const char *name)
+{
+ if (gd->flags & GD_FLG_ENV_READY) { /* after import into hashtable */
+ struct env_entry e, *ep;
+
+ WATCHDOG_RESET();
+
+ e.key = name;
+ e.data = NULL;
+ hsearch_r(e, ENV_FIND, &ep, &env_htab, 0);
+
+ return ep ? ep->data : NULL;
+ }
+
+ /* restricted capabilities before import */
+ if (env_get_f(name, (char *)(gd->env_buf), sizeof(gd->env_buf)) > 0)
+ return (char *)(gd->env_buf);
+
+ return NULL;
+}
+
+/*
+ * Like env_get, but prints an error if envvar isn't defined in the
+ * environment. It always returns what env_get does, so it can be used in
+ * place of env_get without changing error handling otherwise.
+ */
+char *from_env(const char *envvar)
+{
+ char *ret;
+
+ ret = env_get(envvar);
+
+ if (!ret)
+ printf("missing environment variable: %s\n", envvar);
+
+ return ret;
+}
+
+static const char *matching_name_get_value(const char *p, const char *name)
+{
+ while (*name == *p++ && *name != '\0')
+ if (*name++ == '=')
+ return p;
+
+ /* We can look at p[-1] because p was incremented at least once. */
+ if (*name == '\0' && p[-1] == '=')
+ return p;
+
+ return NULL;
+}
+
+/*
+ * Look up variable from environment for restricted C runtime env.
+ */
+int env_get_f(const char *name, char *buf, unsigned len)
+{
+ const char *env, *p, *end;
+
+ if (name == NULL || *name == '\0')
+ return -1;
+
+ if (gd->env_valid == ENV_INVALID)
+ env = (const char *)default_environment;
+ else
+ env = (const char *)gd->env_addr;
+
+ for (p = env; *p != '\0'; p = end + 1) {
+ const char *value;
+ unsigned res;
+
+ for (end = p; *end != '\0'; ++end)
+ if (end - env >= CONFIG_ENV_SIZE)
+ return -1;
+
+ value = matching_name_get_value(p, name);
+ if (value == NULL)
+ continue;
+
+ res = end - value;
+ memcpy(buf, value, min(len, res + 1));
+
+ if (len <= res) {
+ buf[len - 1] = '\0';
+ printf("env_buf [%u bytes] too small for value of \"%s\"\n",
+ len, name);
+ }
+
+ return res;
+ }
+
+ return -1;
+}
+
+/**
+ * Decode the integer value of an environment variable and return it.
+ *
+ * @param name Name of environment 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 env_get_ulong(const char *name, int base, ulong default_val)
+{
+ /*
+ * We can use env_get() here, even before relocation, since the
+ * environment variable value is an integer and thus short.
+ */
+ const char *str = env_get(name);
+
+ return str ? simple_strtoul(str, NULL, base) : default_val;
+}
+
/*
* Read an environment variable as a boolean
* Return -1 if variable does not exist (default to true)
--
2.32.0
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v2 01/13] env: Fix documentation for env_get_f()
2021-10-13 15:45 ` [PATCH v2 01/13] env: Fix documentation for env_get_f() Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> This function actually returns:
> - the number of bytes written into @buf excluding the terminating
> NULL-byte, if there was enough space in @buf
> - the number of bytes written into @buf including the terminating
> NULL-byte, if there wasn't enough space in @buf
> - -1 if the variable is not found
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> include/env.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/env.h b/include/env.h
> index d5e2bcb530..b1a4003681 100644
> --- a/include/env.h
> +++ b/include/env.h
> @@ -131,7 +131,10 @@ char *from_env(const char *envvar);
> * support reading the value (slowly) and some will not.
> *
> * @varname: Variable to look up
> - * @return value of variable, or NULL if not found
> + * @return number of bytes written into @buf, excluding the terminating
> + * NULL-byte if there was enough space in @buf, and including the
> + * terminating NULL-byte if there wasn't enough space, or -1 if the
> + * variable is not found
> */
> int env_get_f(const char *name, char *buf, unsigned int len);
>
> --
> 2.32.0
>
Eek.
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 02/13] env: Drop env_get_char_spec() and old, unused .get_char() implementations
2021-10-13 15:45 ` [PATCH v2 02/13] env: Drop env_get_char_spec() and old, unused .get_char() implementations Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Commit b2cdef4861be ("env: restore old env_get_char() behaviour")
> dropped the .get_char() method from struct env_driver, but left the two
> existing implementations (eeprom and nvram) in case someone would use
> them by overwriting weak function env_get_char_spec().
>
> Since this was never done in the 3.5 years, let's drop these methods and
> simplify the code.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> env/eeprom.c | 18 ------------------
> env/env.c | 7 +------
> env/nvram.c | 14 --------------
> 3 files changed, 1 insertion(+), 38 deletions(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 03/13] examples: api: glue: Remove comment that does not apply anymore
2021-10-13 15:45 ` [PATCH v2 03/13] examples: api: glue: Remove comment that does not apply anymore Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> This comment is not true since commit 6215bd4c1fd6 ("api: Use hashtable
> function for API_env_enum").
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> examples/api/glue.c | 5 -----
> 1 file changed, 5 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 04/13] env: Change env_match() to static and remove from header
2021-10-13 15:45 ` [PATCH v2 04/13] env: Change env_match() to static and remove from header Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
2021-10-14 16:06 ` Marek Behún
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
Hi Marek,
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> This function was used by other parts of U-Boot in the past when
> environment was read from underlying device one character at a time.
>
> This is not the case anymore.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 30 +++++++++++++++---------------
> include/env.h | 11 -----------
> 2 files changed, 15 insertions(+), 26 deletions(-)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index ddc715b4f9..742e0924af 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -706,6 +706,21 @@ char *from_env(const char *envvar)
> return ret;
> }
>
Please can you add the function comment here? We don't want to lose it.
> +static int env_match(uchar *s1, int i2)
> +{
> + if (s1 == NULL)
> + return -1;
> +
> + while (*s1 == env_get_char(i2++))
> + if (*s1++ == '=')
> + return i2;
> +
> + if (*s1 == '\0' && env_get_char(i2-1) == '=')
> + return i2;
> +
> + return -1;
> +}
> +
> /*
> * Look up variable from environment for restricted C runtime env.
> */
> @@ -816,21 +831,6 @@ static int do_env_select(struct cmd_tbl *cmdtp, int flag, int argc,
>
> #endif /* CONFIG_SPL_BUILD */
>
> -int env_match(uchar *s1, int i2)
> -{
> - if (s1 == NULL)
> - return -1;
> -
> - while (*s1 == env_get_char(i2++))
> - if (*s1++ == '=')
> - return i2;
> -
> - if (*s1 == '\0' && env_get_char(i2-1) == '=')
> - return i2;
> -
> - return -1;
> -}
> -
> #ifndef CONFIG_SPL_BUILD
> static int do_env_default(struct cmd_tbl *cmdtp, int flag,
> int argc, char *const argv[])
> diff --git a/include/env.h b/include/env.h
> index b1a4003681..a9b2a4c8b2 100644
> --- a/include/env.h
> +++ b/include/env.h
> @@ -90,17 +90,6 @@ int env_init(void);
> */
> void env_relocate(void);
>
> -/**
> - * env_match() - Match a name / name=value pair
> - *
> - * This is used prior to relocation for finding envrionment variables
> - *
> - * @name: A simple 'name', or a 'name=value' pair.
> - * @index: The environment index for a 'name2=value2' pair.
> - * @return index for the value if the names match, else -1.
> - */
> -int env_match(unsigned char *name, int index);
> -
> /**
> * env_get() - Look up the value of an environment variable
> *
> --
> 2.32.0
>
Regards,
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 05/13] env: Don't match empty variable name in env_match()
2021-10-13 15:45 ` [PATCH v2 05/13] env: Don't match empty variable name in env_match() Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Do we really allow zero-length variable name? I guess not.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 06/13] env: Check for terminating null-byte in env_match()
2021-10-13 15:45 ` [PATCH v2 06/13] env: Check for terminating null-byte " Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> There is a possible overflow in env_match(): if environment contains
> a terminating null-byte before '=' character (i.e. environment is
> broken), the env_match() function can access data after the terminating
> null-byte from parameter pointer.
>
> Example: if env_get_char() returns characters from string array
> "abc\0def\0" and env_match("abc", 0) is called, the function will access
> at least one byte after the end of the "abc" literal.
>
> Fix this by checking for terminating null-byte in env_match().
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> Change since v1:
> - check for '\0' only after incrementing i2
> ---
> cmd/nvedit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 07/13] env: Inline env_get_char() into it's only user
2021-10-13 15:45 ` [PATCH v2 07/13] env: Inline env_get_char() into it's only user Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> This function is a relic from the past when environment was read from
> underlying device one character at a time.
>
> It is used only in the case when getting an environemnt variable prior
> relocation, and the function is simple enough to be inlined there.
>
> Since env_get_char() is being changed to simple access to an array, we
> can drop the failing cases and simplify the code (this could have been
> done before, since env_get_char() did not fail even before).
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 28 ++++++++++++++--------------
> env/env.c | 8 --------
> env/nowhere.c | 5 ++---
> include/env.h | 10 ----------
> 4 files changed, 16 insertions(+), 35 deletions(-)
>
in subject: typo it's should be its
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 08/13] env: Early return from env_get_f() on NULL name
2021-10-13 15:45 ` [PATCH v2 08/13] env: Early return from env_get_f() on NULL name Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Test non-NULL name immediately, not in env_match().
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 09/13] env: Use string pointer instead of indexes in env_get_f()
2021-10-13 15:45 ` [PATCH v2 09/13] env: Use string pointer instead of indexes in env_get_f() Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
2021-10-15 0:40 ` Simon Glass
1 sibling, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Since we no longer use env_get_char() to access n-th character of
> linearized environment data, but rather access the arrays themselves, we
> can convert the iteration to use string pointers instead of position
> indexes.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 10/13] env: Use better name for variable in env_get_f()
2021-10-13 15:45 ` [PATCH v2 10/13] env: Use better name for variable " Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
2021-10-15 0:40 ` Simon Glass
1 sibling, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> The `nxt` variable actually points to the terminating null-byte of the
> current env var, and the next env var is at `nxt + 1`, not `nxt`. So a
> better name for this variable is `end`.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 11/13] env: Make return value of env_get_f() behave like sprintf() on success
2021-10-13 15:45 ` [PATCH v2 11/13] env: Make return value of env_get_f() behave like sprintf() on success Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Currently the env_get_f() function's return value behaves weirdly: it
> returns the number of bytes written into `buf`, but whether this is
> excluding the terminating NULL-byte or including it depends on whether
> there was enough space in `buf`.
>
> Change the function to always return the actual length of the value of
> the environment variable (excluding the terminating NULL-byte) on
> success. This makes it behave like sprintf().
>
> All users of this function in U-Boot are compatible with this change.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 8 +++++---
> include/env.h | 6 ++----
> 2 files changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 12/13] env: Use memcpy() instead of ad-hoc code to copy variable value
2021-10-13 15:45 ` [PATCH v2 12/13] env: Use memcpy() instead of ad-hoc code to copy variable value Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Copy the value of the found variable into given buffer with memcpy()
> instead of ad-hoc code.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 13/13] env: Move non-cmd specific env functions to env/common.c
2021-10-13 15:45 ` [PATCH v2 13/13] env: Move non-cmd specific env functions to env/common.c Marek Behún
@ 2021-10-14 15:11 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-14 15:11 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Move the following functions from cmd/nvedit.c to env/common.c:
> env_set_ulong()
> env_set_hex()
> env_get_hex()
> eth_env_get_enetaddr()
> eth_env_set_enetaddr()
> env_get()
> from_env()
> env_get_f()
> env_get_ulong()
> since these functions are not specific for U-Boot's CLI.
>
> We leave env_set() in cmd/nvedit.c, since it calls _do_env_set(), which
> is a static function in that file.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 185 -------------------------------------------------
> env/common.c | 190 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 190 insertions(+), 185 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
Subject: Move non-cmd-specific env functions//
or maybe Move non-cli env functions...
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 04/13] env: Change env_match() to static and remove from header
2021-10-14 15:11 ` Simon Glass
@ 2021-10-14 16:06 ` Marek Behún
2021-10-14 18:24 ` Simon Glass
0 siblings, 1 reply; 33+ messages in thread
From: Marek Behún @ 2021-10-14 16:06 UTC (permalink / raw)
To: Simon Glass; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Thu, 14 Oct 2021 09:11:08 -0600
Simon Glass <sjg@chromium.org> wrote:
> Hi Marek,
>
> On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
> >
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > This function was used by other parts of U-Boot in the past when
> > environment was read from underlying device one character at a time.
> >
> > This is not the case anymore.
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> > cmd/nvedit.c | 30 +++++++++++++++---------------
> > include/env.h | 11 -----------
> > 2 files changed, 15 insertions(+), 26 deletions(-)
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index ddc715b4f9..742e0924af 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -706,6 +706,21 @@ char *from_env(const char *envvar)
> > return ret;
> > }
> >
>
> Please can you add the function comment here? We don't want to lose
> it.
Simon, the comment is invalid (the function does something
different from what the comment says) and the function is only used as a
helper by env_get_f(), which comes right after it. The function is
refactored and renamend in subsequent patches, and its purpose seems
obvious to me.
Should I really leave the comment there?
Marek
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 04/13] env: Change env_match() to static and remove from header
2021-10-14 16:06 ` Marek Behún
@ 2021-10-14 18:24 ` Simon Glass
2021-10-15 0:40 ` Simon Glass
0 siblings, 1 reply; 33+ messages in thread
From: Simon Glass @ 2021-10-14 18:24 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
Hi Marek,
On Thu, 14 Oct 2021 at 10:06, Marek Behún <kabel@kernel.org> wrote:
>
> On Thu, 14 Oct 2021 09:11:08 -0600
> Simon Glass <sjg@chromium.org> wrote:
>
> > Hi Marek,
> >
> > On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
> > >
> > > From: Marek Behún <marek.behun@nic.cz>
> > >
> > > This function was used by other parts of U-Boot in the past when
> > > environment was read from underlying device one character at a time.
> > >
> > > This is not the case anymore.
> > >
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > ---
> > > cmd/nvedit.c | 30 +++++++++++++++---------------
> > > include/env.h | 11 -----------
> > > 2 files changed, 15 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > > index ddc715b4f9..742e0924af 100644
> > > --- a/cmd/nvedit.c
> > > +++ b/cmd/nvedit.c
> > > @@ -706,6 +706,21 @@ char *from_env(const char *envvar)
> > > return ret;
> > > }
> > >
> >
> > Please can you add the function comment here? We don't want to lose
> > it.
>
> Simon, the comment is invalid (the function does something
> different from what the comment says) and the function is only used as a
> helper by env_get_f(), which comes right after it. The function is
> refactored and renamend in subsequent patches, and its purpose seems
> obvious to me.
>
> Should I really leave the comment there?
That's fine but it would be good to mention that in the commit message
explicitly.
Also you add matching_name_get_value(). Can you add a comment to that, then?
Regards,
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 09/13] env: Use string pointer instead of indexes in env_get_f()
2021-10-13 15:45 ` [PATCH v2 09/13] env: Use string pointer instead of indexes in env_get_f() Marek Behún
2021-10-14 15:11 ` Simon Glass
@ 2021-10-15 0:40 ` Simon Glass
2021-10-15 13:20 ` Marek Behún
1 sibling, 1 reply; 33+ messages in thread
From: Simon Glass @ 2021-10-15 0:40 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
Hi Marek,
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Since we no longer use env_get_char() to access n-th character of
> linearized environment data, but rather access the arrays themselves, we
> can convert the iteration to use string pointers instead of position
> indexes.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 7c99a693ea..6eabd51209 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -706,17 +706,17 @@ char *from_env(const char *envvar)
> return ret;
> }
>
> -static int env_match(const char *env, const char *s1, int i2)
> +static const char *matching_name_get_value(const char *p, const char *name)
OK so this is the function where I would like a nice comment, please.
> {
> - while (*s1 == env[i2++] && *s1 != '\0')
> - if (*s1++ == '=')
> - return i2;
> + while (*name == *p++ && *name != '\0')
> + if (*name++ == '=')
> + return p;
>
> - /* We can look at env[i2-1] because i2 was incremented at least once. */
> - if (*s1 == '\0' && env[i2-1] == '=')
> - return i2;
> + /* We can look at p[-1] because p was incremented at least once. */
> + if (*name == '\0' && p[-1] == '=')
> + return p;
>
> - return -1;
> + return NULL;
> }
>
> /*
> @@ -724,8 +724,7 @@ static int env_match(const char *env, const char *s1, int i2)
> */
> int env_get_f(const char *name, char *buf, unsigned len)
> {
> - const char *env;
> - int i, nxt;
> + const char *env, *p, *nxt;
>
> if (name == NULL || *name == '\0')
> return -1;
> @@ -735,20 +734,21 @@ int env_get_f(const char *name, char *buf, unsigned len)
> else
> env = (const char *)gd->env_addr;
>
> - for (i = 0; env[i] != '\0'; i = nxt + 1) {
> - int val, n;
> + for (p = env; *p != '\0'; p = nxt + 1) {
> + const char *value;
> + int n;
>
> - for (nxt = i; env[nxt] != '\0'; ++nxt)
> - if (nxt >= CONFIG_ENV_SIZE)
> + for (nxt = p; *nxt != '\0'; ++nxt)
> + if (nxt - env >= CONFIG_ENV_SIZE)
> return -1;
>
> - val = env_match(env, name, i);
> - if (val < 0)
> + value = matching_name_get_value(p, name);
> + if (value == NULL)
> continue;
>
> /* found; copy out */
> for (n = 0; n < len; ++n, ++buf) {
> - *buf = env[val++];
> + *buf = *value++;
> if (*buf == '\0')
> return n;
> }
> --
> 2.32.0
>
Regards,
Simon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 10/13] env: Use better name for variable in env_get_f()
2021-10-13 15:45 ` [PATCH v2 10/13] env: Use better name for variable " Marek Behún
2021-10-14 15:11 ` Simon Glass
@ 2021-10-15 0:40 ` Simon Glass
1 sibling, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-15 0:40 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
Hi Marek,
On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> The `nxt` variable actually points to the terminating null-byte of the
> current env var, and the next env var is at `nxt + 1`, not `nxt`. So a
> better name for this variable is `end`.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> cmd/nvedit.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 6eabd51209..08288fad10 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -724,7 +724,7 @@ static const char *matching_name_get_value(const char *p, const char *name)
> */
> int env_get_f(const char *name, char *buf, unsigned len)
> {
> - const char *env, *p, *nxt;
> + const char *env, *p, *end;
>
> if (name == NULL || *name == '\0')
> return -1;
> @@ -734,12 +734,12 @@ int env_get_f(const char *name, char *buf, unsigned len)
> else
> env = (const char *)gd->env_addr;
>
> - for (p = env; *p != '\0'; p = nxt + 1) {
> + for (p = env; *p != '\0'; p = end + 1) {
> const char *value;
> int n;
>
> - for (nxt = p; *nxt != '\0'; ++nxt)
> - if (nxt - env >= CONFIG_ENV_SIZE)
> + for (end = p; *end != '\0'; ++end)
> + if (end - env >= CONFIG_ENV_SIZE)
> return -1;
>
> value = matching_name_get_value(p, name);
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 04/13] env: Change env_match() to static and remove from header
2021-10-14 18:24 ` Simon Glass
@ 2021-10-15 0:40 ` Simon Glass
0 siblings, 0 replies; 33+ messages in thread
From: Simon Glass @ 2021-10-15 0:40 UTC (permalink / raw)
To: Marek Behún; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
On Thu, 14 Oct 2021 at 12:24, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Marek,
>
> On Thu, 14 Oct 2021 at 10:06, Marek Behún <kabel@kernel.org> wrote:
> >
> > On Thu, 14 Oct 2021 09:11:08 -0600
> > Simon Glass <sjg@chromium.org> wrote:
> >
> > > Hi Marek,
> > >
> > > On Wed, 13 Oct 2021 at 09:46, Marek Behún <kabel@kernel.org> wrote:
> > > >
> > > > From: Marek Behún <marek.behun@nic.cz>
> > > >
> > > > This function was used by other parts of U-Boot in the past when
> > > > environment was read from underlying device one character at a time.
> > > >
> > > > This is not the case anymore.
> > > >
> > > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > > ---
> > > > cmd/nvedit.c | 30 +++++++++++++++---------------
> > > > include/env.h | 11 -----------
> > > > 2 files changed, 15 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > > > index ddc715b4f9..742e0924af 100644
> > > > --- a/cmd/nvedit.c
> > > > +++ b/cmd/nvedit.c
> > > > @@ -706,6 +706,21 @@ char *from_env(const char *envvar)
> > > > return ret;
> > > > }
> > > >
> > >
> > > Please can you add the function comment here? We don't want to lose
> > > it.
> >
> > Simon, the comment is invalid (the function does something
> > different from what the comment says) and the function is only used as a
> > helper by env_get_f(), which comes right after it. The function is
> > refactored and renamend in subsequent patches, and its purpose seems
> > obvious to me.
> >
> > Should I really leave the comment there?
>
> That's fine but it would be good to mention that in the commit message
> explicitly.
>
> Also you add matching_name_get_value(). Can you add a comment to that, then?
>
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 09/13] env: Use string pointer instead of indexes in env_get_f()
2021-10-15 0:40 ` Simon Glass
@ 2021-10-15 13:20 ` Marek Behún
0 siblings, 0 replies; 33+ messages in thread
From: Marek Behún @ 2021-10-15 13:20 UTC (permalink / raw)
To: Simon Glass; +Cc: Tom Rini, U-Boot Mailing List, Marek Behún
Hi Simon,
On Thu, 14 Oct 2021 18:40:02 -0600
Simon Glass <sjg@chromium.org> wrote:
> > -static int env_match(const char *env, const char *s1, int i2)
> > +static const char *matching_name_get_value(const char *p, const
> > char *name)
>
> OK so this is the function where I would like a nice comment, please.
Now that I look at it, I notice that it also works for cases when name
is "abc=def" (it matches just "abc"). This was the purpose of
env_match() before, but env_get_f() does not use it that way, and this
functionality can now be removed.
This would simplify the code to something like
len = strlen(name);
if (p[len] == '=' && !strncmp(p, name, len))
return &p[len + 1];
return NULL;
And this is simple enough to be inlined into env_get_f().
I will refactor this and send v3.
Marek
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2021-10-15 13:20 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-13 15:45 [PATCH v2 00/13] env_get_char() removal and env_get_f() refactor Marek Behún
2021-10-13 15:45 ` [PATCH v2 01/13] env: Fix documentation for env_get_f() Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 02/13] env: Drop env_get_char_spec() and old, unused .get_char() implementations Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 03/13] examples: api: glue: Remove comment that does not apply anymore Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 04/13] env: Change env_match() to static and remove from header Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-14 16:06 ` Marek Behún
2021-10-14 18:24 ` Simon Glass
2021-10-15 0:40 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 05/13] env: Don't match empty variable name in env_match() Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 06/13] env: Check for terminating null-byte " Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 07/13] env: Inline env_get_char() into it's only user Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 08/13] env: Early return from env_get_f() on NULL name Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 09/13] env: Use string pointer instead of indexes in env_get_f() Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-15 0:40 ` Simon Glass
2021-10-15 13:20 ` Marek Behún
2021-10-13 15:45 ` [PATCH v2 10/13] env: Use better name for variable " Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-15 0:40 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 11/13] env: Make return value of env_get_f() behave like sprintf() on success Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 12/13] env: Use memcpy() instead of ad-hoc code to copy variable value Marek Behún
2021-10-14 15:11 ` Simon Glass
2021-10-13 15:45 ` [PATCH v2 13/13] env: Move non-cmd specific env functions to env/common.c Marek Behún
2021-10-14 15:11 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.