Re: [U-Boot] [PATCH v1 3/5] env: unify logic to check and apply changes

Hi,

On Fri, Nov 18, 2011 at 8:49 AM, Gerlando Falauto gerlando.falauto@keymile.com wrote:

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

I haven't seen any comments on this series, but it seems good to me. However it no longer applies - can you please rebase and resend?

Regards, Simon

---

common/cmd_nvedit.c   |  163 +++++++++++++++++++++++++++++++-----------------  common/env_common.c   |    6 +-  include/environment.h |    7 ++  include/search.h      |   17 +++++-  lib/hashtable.c       |   43 +++++++++++++-  5 files changed, 174 insertions(+), 62 deletions(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index 7194ade..d6ea25d 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -196,32 +196,23 @@ static int do_env_grep (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[  #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) @@ -232,22 +223,23 @@ int _do_env_set (int flag, int argc, char * const argv[])                console = stderr;

if (console != -1) {

• if (argc < 3) {         /* Cannot delete it! */

• if ((newval == NULL) || (*newval == '\0')) {
• /* We cannot delete stdin/stdout/stderr */

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 */ @@ -255,25 +247,35 @@ 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]) @@ -284,6 +286,10 @@ int _do_env_set (int flag, int argc, char * const argv[])                                        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);                        udelay(50000); @@ -301,6 +307,74 @@ 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(newval, "loadaddr") == 0) {
• load_addr = simple_strtoul(newval, NULL, 16);
• return 0;
• }

+#if defined(CONFIG_CMD_NET)

• else if (strcmp(newval, "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); @@ -338,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;  }

@@ -888,7 +934,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv                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 c7e9bea..9d8c59e 100644 --- a/common/env_common.c +++ b/common/env_common.c @@ -190,7 +190,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; @@ -215,7 +216,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 6394a96..1936411 100644 --- a/include/environment.h +++ b/include/environment.h @@ -174,6 +174,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

#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 b7ba341..22421f9 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

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot