[U-Boot] [PATCH v2 1/3] env: unify logic to check and apply changes

Gerlando Falauto
2011-12-07T13:30:03Z

The logic of checking special parameters (e.g. baudrate, stdin, stdout,
for a valid value and/or whether can be overwritten) and applying the
new value to the running system is now all within a single function
env_check_apply() which can be called whenever changes are made
to the environment, no matter if by set, default or import.

With this patch env_check_apply() is only called by "env set",
retaining previous behavior.

Also allow for selectively importing/resetting variables.

So add 3 new arguments to himport_r():
o "nvars", "vars":, number and list of variables to take into account
(0 means ALL)

o "apply" callback function to check whether a variable can be
overwritten, and possibly immediately apply the changes;
when NULL, no check is performed.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
---
common/cmd_nvedit.c | 174 +++++++++++++++++++++++++++++++------------------
common/env_common.c | 6 +-
include/environment.h | 7 ++
include/search.h | 17 +++++-
lib/hashtable.c | 43 ++++++++++++-
5 files changed, 180 insertions(+), 67 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 5995354..a31d413 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -197,32 +197,23 @@ static int do_env_grep(cmd_tbl_t *cmdtp, int flag,
#endif

/*
- * Set a new environment variable,
- * or replace or delete an existing one.
+ * Performs consistency checking before setting, replacing,
+ * or deleting an environment variable, then (if successful)
+ * apply the changes to internals so to make them effective.
+ * Code for this function was taken out of _do_env_set(),
+ * which now calls it.
+ * Also called as a callback function by himport_r().
+ * Returns 0 in case of success, 1 in case of failure.
+ * When (flag & H_FORCE) is set, force overwriting of
+ * write-once variables.
*/
-int _do_env_set(int flag, int argc, char * const argv[])
+
+int env_check_apply(const char *name, const char *oldval,
+ const char *newval, int flag)
{
bd_t *bd = gd->bd;
- int i, len;
+ int i;
int console = -1;
- char *name, *value, *s;
- ENTRY e, *ep;
-
- name = argv[1];
-
- if (strchr(name, '=')) {
- printf("## Error: illegal character '=' in variable name"
- "\"%s\"\n", name);
- return 1;
- }
-
- env_id++;
- /*
- * search if variable with this name already exists
- */
- e.key = name;
- e.data = NULL;
- hsearch_r(e, FIND, &ep, &env_htab);

/* Check for console redirection */
if (strcmp(name, "stdin") == 0)
@@ -233,22 +224,24 @@ int _do_env_set(int flag, int argc, char * const argv[])
console = stderr;

if (console != -1) {
- if (argc < 3) { /* Cannot delete it! */
- printf("Can't delete \"%s\"\n", name);
+ if ((newval == NULL) || (*newval == '\0')) {
+ /* We cannot delete stdin/stdout/stderr */
+ if ((flag & H_FORCE) == 0)
+ printf("Can't delete \"%s\"\n", name);
return 1;
}

#ifdef CONFIG_CONSOLE_MUX
- i = iomux_doenv(console, argv[2]);
+ i = iomux_doenv(console, newval);
if (i)
return i;
#else
/* Try assigning specified device */
- if (console_assign(console, argv[2]) < 0)
+ if (console_assign(console, newval) < 0)
return 1;

#ifdef CONFIG_SERIAL_MULTI
- if (serial_assign(argv[2]) < 0)
+ if (serial_assign(newval) < 0)
return 1;
#endif
#endif /* CONFIG_CONSOLE_MUX */
@@ -256,37 +249,52 @@ int _do_env_set(int flag, int argc, char * const argv[])

/*
* Some variables like "ethaddr" and "serial#" can be set only
- * once and cannot be deleted; also, "ver" is readonly.
+ * once and cannot be deleted, unless CONFIG_ENV_OVERWRITE
+ * is defined.
*/
- if (ep) { /* variable exists */
#ifndef CONFIG_ENV_OVERWRITE
+ if (oldval != NULL && /* variable exists */
+ (flag & H_FORCE) == 0) { /* and we are not forced */
if (strcmp(name, "serial#") == 0 ||
(strcmp(name, "ethaddr") == 0
#if defined(CONFIG_OVERWRITE_ETHADDR_ONCE) && defined(CONFIG_ETHADDR)
- && strcmp(ep->data, MK_STR(CONFIG_ETHADDR)) != 0
+ && strcmp(oldval, MK_STR(CONFIG_ETHADDR)) != 0
#endif /* CONFIG_OVERWRITE_ETHADDR_ONCE && CONFIG_ETHADDR */
)) {
printf("Can't overwrite \"%s\"\n", name);
return 1;
}
+ }
#endif
+ /*
+ * When we change baudrate, or we are doing an env default -a
+ * (which will erase all variables prior to calling this),
+ * we want the baudrate to actually change - for real.
+ */
+ if (oldval != NULL || /* variable exists */
+ (flag & H_NOCLEAR) == 0) { /* or env is clear */
/*
* Switch to new baudrate if new baudrate is supported
*/
if (strcmp(name, "baudrate") == 0) {
- int baudrate = simple_strtoul(argv[2], NULL, 10);
+ int baudrate = simple_strtoul(newval, NULL, 10);
int i;
for (i = 0; i < N_BAUDRATES; ++i) {
if (baudrate == baudrate_table[i])
break;
}
if (i == N_BAUDRATES) {
- printf("## Baudrate %d bps not supported\n",
- baudrate);
+ if ((flag & H_FORCE) == 0)
+ printf("## Baudrate %d bps not "
+ "supported\n", baudrate);
return 1;
}
+ if (gd->baudrate == baudrate) {
+ /* If unchanged, we just say it's OK */
+ return 0;
+ }
printf("## Switch baudrate to %d bps and"
- "press ENTER ...\n", baudrate);
+ "press ENTER ...\n", baudrate);
udelay(50000);
gd->baudrate = baudrate;
#if defined(CONFIG_PPC) || defined(CONFIG_MCF52x2)
@@ -300,6 +308,73 @@ int _do_env_set(int flag, int argc, char * const argv[])
}
}

+ /*
+ * Some variables should be updated when the corresponding
+ * entry in the environment is changed
+ */
+ if (strcmp(name, "ipaddr") == 0) {
+ const char *s = newval;
+ char *e;
+ unsigned long addr;
+ bd->bi_ip_addr = 0;
+ for (addr = 0, i = 0; i < 4; ++i) {
+ ulong val = s ? simple_strtoul(s, &e, 10) : 0;
+ addr <<= 8;
+ addr |= val & 0xFF;
+ if (s)
+ s = *e ? e + 1 : e;
+ }
+ bd->bi_ip_addr = htonl(addr);
+ return 0;
+ } else if (strcmp(name, "loadaddr") == 0) {
+ load_addr = simple_strtoul(newval, NULL, 16);
+ return 0;
+ }
+#if defined(CONFIG_CMD_NET)
+ else if (strcmp(name, "bootfile") == 0) {
+ copy_filename(BootFile, newval, sizeof(BootFile));
+ return 0;
+ }
+#endif
+ return 0;
+}
+
+/*
+ * Set a new environment variable,
+ * or replace or delete an existing one.
+*/
+int _do_env_set(int flag, int argc, char * const argv[])
+{
+ int i, len;
+ char *name, *value, *s;
+ ENTRY e, *ep;
+
+ name = argv[1];
+ value = argv[2];
+
+ if (strchr(name, '=')) {
+ printf("## Error: illegal character '='"
+ "in variable name \"%s\"\n", name);
+ return 1;
+ }
+
+ env_id++;
+ /*
+ * search if variable with this name already exists
+ */
+ e.key = name;
+ e.data = NULL;
+ hsearch_r(e, FIND, &ep, &env_htab);
+
+ /*
+ * Perform requested checks. Notice how since we are overwriting
+ * a single variable, we need to set H_NOCLEAR
+ */
+ if (env_check_apply(name, ep ? ep->data : NULL, value, H_NOCLEAR)) {
+ debug("check function did not approve, refusing\n");
+ return 1;
+ }
+
/* Delete only ? */
if (argc < 3 || argv[2] == NULL) {
int rc = hdelete_r(name, &env_htab);
@@ -337,34 +412,6 @@ int _do_env_set(int flag, int argc, char * const argv[])
return 1;
}

- /*
- * Some variables should be updated when the corresponding
- * entry in the environment is changed
- */
- if (strcmp(name, "ipaddr") == 0) {
- char *s = argv[2]; /* always use only one arg */
- char *e;
- unsigned long addr;
- bd->bi_ip_addr = 0;
- for (addr = 0, i = 0; i < 4; ++i) {
- ulong val = s ? simple_strtoul(s, &e, 10) : 0;
- addr <<= 8;
- addr |= val & 0xFF;
- if (s)
- s = *e ? e + 1 : e;
- }
- bd->bi_ip_addr = htonl(addr);
- return 0;
- } else if (strcmp(argv[1], "loadaddr") == 0) {
- load_addr = simple_strtoul(argv[2], NULL, 16);
- return 0;
- }
-#if defined(CONFIG_CMD_NET)
- else if (strcmp(argv[1], "bootfile") == 0) {
- copy_filename(BootFile, argv[2], sizeof(BootFile));
- return 0;
- }
-#endif
return 0;
}

@@ -886,7 +933,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
addr = (char *)ep->data;
}

- if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == 0) {
+ if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR,
+ 0, NULL, NULL) == 0) {
error("Environment import failed: errno = %d\n", errno);
return 1;
}
diff --git a/common/env_common.c b/common/env_common.c
index 8a71096..7e2bb2f 100644
--- a/common/env_common.c
+++ b/common/env_common.c
@@ -175,7 +175,8 @@ void set_default_env(const char *s)
}

if (himport_r(&env_htab, (char *)default_environment,
- sizeof(default_environment), '\0', 0) == 0)
+ sizeof(default_environment), '\0', 0,
+ 0, NULL, NULL) == 0)
error("Environment import failed: errno = %d\n", errno);

gd->flags |= GD_FLG_ENV_READY;
@@ -200,7 +201,8 @@ int env_import(const char *buf, int check)
}
}

- if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) {
+ if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0,
+ 0, NULL, NULL)) {
gd->flags |= GD_FLG_ENV_READY;
return 1;
}
diff --git a/include/environment.h b/include/environment.h
index 3c145af..3a3e6b8 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -193,6 +193,13 @@ void set_default_env(const char *s);
/* Import from binary representation into hash table */
int env_import(const char *buf, int check);

+/*
+ * Check if variable "name" can be changed from oldval to newval,
+ * and if so, apply the changes (e.g. baudrate)
+ */
+int env_check_apply(const char *name, const char *oldval,
+ const char *newval, int flag);
+
#endif /* DO_DEPS_ONLY */

#endif /* _ENVIRONMENT_H_ */
diff --git a/include/search.h b/include/search.h
index ef53edb..2a59e03 100644
--- a/include/search.h
+++ b/include/search.h
@@ -47,6 +47,13 @@ typedef struct entry {
struct _ENTRY;

/*
+ * Callback function to be called for checking whether the given change may
+ * be applied or not. Must return 0 for approval, 1 for denial.
+ */
+typedef int (*apply_cb)(const char *name, const char *oldval,
+ const char *newval, int flag);
+
+/*
* Family of hash table handling functions. The functions also
* have reentrant counterparts ending with _r. The non-reentrant
* functions all work on a signle internal hashing table.
@@ -94,11 +101,19 @@ extern ssize_t hexport_r(struct hsearch_data *__htab,
const char __sep, char **__resp, size_t __size,
int argc, char * const argv[]);

+/*
+ * nvars, vars: variables to import (nvars == 0 means all)
+ * apply_cb: callback function to check validity of the new argument,
+ * and possibly apply changes (NULL means accept everything)
+ */
extern int himport_r(struct hsearch_data *__htab,
const char *__env, size_t __size, const char __sep,
- int __flag);
+ int __flag,
+ int nvars, char * const vars[],
+ apply_cb apply);

/* Flags for himport_r() */
#define H_NOCLEAR 1 /* do not clear hash table before importing */
+#define H_FORCE 2 /* overwrite read-only/write-once variables */

#endif /* search.h */
diff --git a/lib/hashtable.c b/lib/hashtable.c
index abd61c8..75b9b07 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep,
* himport()
*/

+/* Check whether variable name is amongst vars[] */
+static int process_var(const char *name, int nvars, char * const vars[])
+{
+ int i = 0;
+ /* No variables specified means process all of them */
+ if (nvars == 0)
+ return 1;
+
+ for (i = 0; i < nvars; i++) {
+ if (!strcmp(name, vars[i]))
+ return 1;
+ }
+ debug("Skipping non-listed variable %s\n", name);
+ return 0;
+}
+
/*
* Import linearized data into hash table.
*
@@ -639,7 +655,9 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep,
*/

int himport_r(struct hsearch_data *htab,
- const char *env, size_t size, const char sep, int flag)
+ const char *env, size_t size, const char sep, int flag,
+ int nvars, char * const vars[],
+ apply_cb apply)
{
char *data, *sp, *dp, *name, *value;

@@ -726,6 +744,8 @@ int himport_r(struct hsearch_data *htab,
*dp++ = '\0'; /* terminate name */

debug("DELETE CANDIDATE: \"%s\"\n", name);
+ if (!process_var(name, nvars, vars))
+ continue;

if (hdelete_r(name, htab) == 0)
debug("DELETE ERROR ##############################\n");
@@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab,
*sp++ = '\0'; /* terminate value */
++dp;

+ /* Skip variables which are not supposed to be treated */
+ if (!process_var(name, nvars, vars))
+ continue;
+
/* enter into hash table */
e.key = name;
e.data = value;

+ /* if there is an apply function, check what it has to say */
+ if (apply != NULL) {
+ debug("searching before calling cb function"
+ " for %s\n", name);
+ /*
+ * Search for variable in existing env, so to pass
+ * its previous value to the apply callback
+ */
+ hsearch_r(e, FIND, &rv, htab);
+ debug("previous value was %s\n", rv ? rv->data : "");
+ if (apply(name, rv ? rv->data : NULL, value, flag)) {
+ debug("callback function refused to set"
+ " variable %s, skipping it!\n", name);
+ continue;
+ }
+ }
+
hsearch_r(e, ENTER, &rv, htab);
if (rv == NULL) {
printf("himport_r: can't insert \"%s=%s\" into hash table\n",
--
1.7.1