env: handle special variables and selective env default

Signed-off-by Gerlando Falauto <[email protected]>

---
 common/serial.c  |   51 +++++++++++++++++++++++++--------------------------
 include/serial.h |   19 ++++++++++---------
 2 files changed, 35 insertions(+), 35 deletions(-)

Signed-off-by Gerlando Falauto <[email protected]>

---
 common/serial.c  |    2 +-
 include/serial.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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 <[email protected]>

---

?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(-)

common/cmd_nvedit.c +0 -0

index 7194ade..d6ea25d 100644
@@ -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.
?*/

common/cmd_nvedit.c +2 -1

-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;

common/cmd_nvedit.c +0 -15

-
- ? ? ? 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)

common/cmd_nvedit.c +2 -1

@@ -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 */

common/cmd_nvedit.c +0 -0

@@ -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])

common/cmd_nvedit.c +72 -0

@@ -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);

common/cmd_nvedit.c +2 -29

@@ -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;
 ? ? ? ?}

common/env_common.c +4 -2

index c7e9bea..9d8c59e 100644
@@ -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;
 ? ? ? ?}

include/environment.h +0 -0

index 6394a96..1936411 100644
@@ -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);

include/environment.h +7 -0

+/*
+ * 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_ */

include/search.h +0 -0

index ef53edb..2a59e03 100644
@@ -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.
+ */

include/search.h +4 -0

+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.

include/search.h +8 -1

@@ -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 */

lib/hashtable.c +0 -0

index b7ba341..22421f9 100644
@@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep,
 ?* himport()

?*/

lib/hashtable.c +16 -0

+/* 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.
?*

lib/hashtable.c +0 -0

@@ -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;

lib/hashtable.c +0 -0

@@ -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");

lib/hashtable.c +0 -0

@@ -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 at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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 <[email protected]>

---

?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(-)

common/cmd_nvedit.c +0 -0

index 5995354..a31d413 100644
@@ -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.

include/environment.h +0 -0

index 3c145af..3a3e6b8 100644
@@ -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)

include/environment.h +0 -0

@@ -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);

include/environment.h +2 -0

+
+/*

?* 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.

include/environment.h +3 -1

 ?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);
 
 

lib/hashtable.c +0 -0

index abd61c8..75b9b07 100644
@@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep,
 ?* himport()

?*/

+ ? ? ? /* 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;

lib/hashtable.c +2 -0

+}
+

?/*
?* Import linearized data into hash table.
?*

lib/hashtable.c +0 -0

@@ -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 */

Also, for me this patch adds 500 bytes. I wonder if more of the code could made optional?

Quoted 4 lines

/*
- * Set a new environment variable,
- * or replace or delete an existing one.
+ * Performs consistency checking before setting, replacing,

Quoted 8 lines

+ * 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.

can you word-wrap that to 72 columns perhaps?

lib/hashtable.c ssize_t hexport_r(struct hsearch_data *htab, const char sep, · line 609

Quoted 4 lines

lib/hashtable.c +4 -0

+/* Check whether variable name is amongst vars[] */
+static int process_var(const char *name, int nvars, char * const vars[])
+{
+       int i = 0;

blank line here.

Can part of this function be #ifdefed to reduce code size when the feature is not required?

lib/hashtable.c +2 -0

+                       if (!process_var(name, nvars, vars))
+                               continue;
 
                        if (hdelete_r(name, htab) == 0)
                                debug("DELETE ERROR

##############################\n");

perhaps:

if (process_var(name, nvars, vars)&&

            hdelete_r(name, htab) == 0)
      debug("DELETE ERROR ##############################\n");

himport_r() is getting a bit overloaded,

and it's a shame but I can't
think of an easy way to refactor it to reduce the number of args. In a way you are adding a processing option and so you could put the three extra args in a structure and pass a pointer to it (or NULL to skip this feature). Not sure though...

Also, for me this patch adds 500 bytes. I wonder if more of the code could made optional?

Quoted 4 lines

if (process_var(name, nvars, vars)&&

            hdelete_r(name, htab) == 0)
      debug("DELETE ERROR ##############################\n");

I think it's easier to read it the original way, and it should not make any difference as far as code size is concerned.

Quoted 1 line

himport_r() is getting a bit overloaded,

Actually, I believe it makes no longer sense to have it called "_r", as it was the original reference to the function being recursively calleable (i.e. reentrant) as opposed to other versions which were not.

Quoted 2 lines

Also, for me this patch adds 500 bytes. I wonder if more of the code could made optional?

Frankly, I'm surprised to hear this adds that much overhead. Actually, I can't see this increase in code size as you mention. What architecture are you referring to?
In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are surprisingly unchanged in size, even with debug #defined!

Dear Gerlando Falauto,

Quoted 2 lines

I think it's easier to read it the original way, and it should not make any difference as far as code size is concerned.

The Coding Style makes an explicit exception regarding the line length for user visible strings:

83 Statements longer than 80 columns will be broken into sensible chunks, unless 84 exceeding 80 columns significantly increases readability and does not hide 85 information. Descendants are always substantially shorter than the parent and 86 are placed substantially to the right. The same applies to function headers 87 with a long argument list. However, never break user-visible strings such as 88 printk messages, because that breaks the ability to grep for them.

Quoted 5 lines

Quoted 1 line

himport_r() is getting a bit overloaded,

Actually, I believe it makes no longer sense to have it called "_r", as it was the original reference to the function being recursively calleable (i.e. reentrant) as opposed to other versions which were not.

Has this changed?

Quoted 8 lines

Quoted 2 lines

Also, for me this patch adds 500 bytes. I wonder if more of the code could made optional?

Frankly, I'm surprised to hear this adds that much overhead. Actually, I can't see this increase in code size as you mention. What architecture are you referring to?
In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are surprisingly unchanged in size, even with debug #defined!

Don't look at file size. Use the "size" command and compare code / data / bss sizes.

I don't understand: why are you quoting this here and now? The chunk we are referring to doesn't change a bit about the printable string.

On 12/07/2011 11:02 PM, Simon Glass wrote:

[...]

Quoted 15 lines

Quoted 13 lines

?/*
- * 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.

Quoted 1 line

can you word-wrap that to 72 columns perhaps?

Sorry, I am little confused now. What is the maximum line length?

[...]

Quoted 7 lines

lib/hashtable.c ssize_t hexport_r(struct hsearch_data *htab, const char sep,

Quoted 4 lines

lib/hashtable.c +4 -0

+/* Check whether variable name is amongst vars[] */
+static int process_var(const char *name, int nvars, char * const vars[])
+{
+ ? ? ? int i = 0;

blank line here.

Thanks, I didn't know about this.

Quoted 2 lines

Can part of this function be #ifdefed to reduce code size when the feature is not required?

Uhm, I don't think so. This would be a common feature for selectively importing & setting back to default. Unless we want to #ifdef both of them. Personally, I would #ifdef neither.

[...]

Quoted 10 lines

+ ? ? ? ? ? ? ? ? ? ? ? if (!process_var(name, nvars, vars))
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? continue;

? ? ? ? ? ? ? ? ? ? ? ?if (hdelete_r(name, htab) == 0) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?debug("DELETE ERROR ##############################\n");

perhaps:

Quoted 3 lines

if (process_var(name, nvars, vars)&&
? ? ? ? ? ?hdelete_r(name, htab) == 0)
? ? ?debug("DELETE ERROR ##############################\n");

I think it's easier to read it the original way, and it should not make any difference as far as code size is concerned.

Quoted 3 lines

himport_r() is getting a bit overloaded,

Actually, I believe it makes no longer sense to have it called "_r", as it was the original reference to the function being recursively calleable (i.e. reentrant) as opposed to other versions which were not.

Quoted 7 lines

and it's a shame but I can't
think of an easy way to refactor it to reduce the number of args. In a way you are adding a processing option and so you could put the three extra args in a structure and pass a pointer to it (or NULL to skip this feature). Not sure though...

Can't think of any other way either, except maybe renaming it and re-implementing the shorter version as a wrapper around the newly named function. I had already done that, but there would be very few places where the old syntax would be kept, so it just didn't make much sense.

Quoted 4 lines

Also, for me this patch adds 500 bytes. I wonder if more of the code could made optional?

Frankly, I'm surprised to hear this adds that much overhead. Actually, I can't see this increase in code size as you mention. What architecture are you referring to?

In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are surprisingly unchanged in size, even with debug #defined! Only place I can experience such growth is within unstripped u-boot ELF, which I believe shouldn't really matter... or should it?

Best,
Gerlando

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 <[email protected]>

common/cmd_nvedit.c +44 -29

+		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;
 	}

common/env_common.c +4 -2

index 8a71096..7e2bb2f 100644
@@ -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;
 	}

include/environment.h +7 -0

index 3c145af..3a3e6b8 100644
@@ -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_ */

include/search.h +1 -0

index ef53edb..2a59e03 100644
@@ -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.
+ */

include/search.h +6 -0

+
+/*
  * 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)

lib/hashtable.c +14 -0

+{
+	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,

lib/hashtable.c +3 -1

-	      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;

lib/hashtable.c +10 -0

+		/* 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 : "");

Dear Gerlando Falauto,

WD prodded me too long to review this patchset ;-D

common/cmd_nvedit.c int _do_env_set(int flag, int argc, char * const argv[]) · line 335

Quoted 3 lines

common/cmd_nvedit.c +3 -0

+#if defined(CONFIG_CMD_NET)
+	else if (strcmp(name, "bootfile") == 0) {
+		copy_filename(BootFile, newval, sizeof(BootFile));

Can you remove the camel-case here please?

Quoted 6 lines

common/cmd_nvedit.c +6 -0

+		return 0;
+	}
+#endif
+	return 0;
+}
+

include/search.h typedef struct entry { · line 54

Quoted 11 lines

include/search.h +1 -0

@@ -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.
+ */

include/search.h +2 -0

+typedef int (*apply_cb)(const char *name, const char *oldval,
+			const char *newval, int flag);

Is the typedef really necessary ?

[From your other email]

I have to admit I'm not much of a fan of how you use this apply() callback, is it really necessary?

Quoted 3 lines

/* Flags for himport_r() */
#define H_NOCLEAR 1 /* do not clear hash table before

importing */

Quoted 1 line

+#define H_FORCE 2 /* overwrite read-only/write-once

variables */

Make this 1<< x please.

Quoted 11 lines

  #endif /* search.h */

lib/hashtable.c +0 -0

index abd61c8..75b9b07 100644
@@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const
 char sep, * himport()
   */
 

lib/hashtable.c ssize_t hexport_r(struct hsearch_data *htab, const char sep, · line 607

Quoted 2 lines

lib/hashtable.c +2 -0

+/* Check whether variable name is amongst vars[] */
+static int process_var(const char *name, int nvars, char * const vars[])

You mean check_var()?

Quoted 29 lines

lib/hashtable.c +14 -0

+{
+	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,

lib/hashtable.c +3 -1

-	      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;

lib/hashtable.c int himport_r(struct hsearch_data *htab, · line 751

Quoted 8 lines

lib/hashtable.c +2 -0

@@ -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");

Quoted 4 lines

lib/hashtable.c +0 -0

@@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab,
  		*sp++ = '\0';	/* terminate value */
  		++dp;
 

lib/hashtable.c int himport_r(struct hsearch_data *htab, · line 772

Quoted 7 lines

lib/hashtable.c +4 -0

+		/* 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;

Do you need to do this if you eventually later figure out you have no apply() callback and you did this for no reason?

Quoted 14 lines

lib/hashtable.c +13 -0

+		/* 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);

lib/hashtable.c int himport_r(struct hsearch_data *htab,

Quoted 7 lines

lib/hashtable.c +4 -0

+				continue;
+			}
+		}
+
  		hsearch_r(e, ENTER,&rv, htab);
  		if (rv == NULL) {
  			printf("himport_r: can't insert \"%s=%s\" into hash

table\n",

Quoted 3 lines

Dear Gerlando Falauto,

WD prodded me too long to review this patchset ;-D

Well, better late than never! ;-)

[...]

Quoted 5 lines

common/cmd_nvedit.c int _do_env_set(int flag, int argc, char * const argv[]) · line 335

Quoted 3 lines

common/cmd_nvedit.c +3 -0

+#if defined(CONFIG_CMD_NET)
+	else if (strcmp(name, "bootfile") == 0) {
+		copy_filename(BootFile, newval, sizeof(BootFile));

Can you remove the camel-case here please?

That's code I just moved around... Will do, sir.

Quoted 6 lines

Quoted 6 lines

common/cmd_nvedit.c +6 -0

+		return 0;
+	}
+#endif
+	return 0;
+}
+

[...]

Quoted 16 lines

include/search.h typedef struct entry { · line 54

Quoted 13 lines

include/search.h +1 -0

@@ -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.
+ */

include/search.h +2 -0

+typedef int (*apply_cb)(const char *name, const char *oldval,
+			const char *newval, int flag);

Is the typedef really necessary ?

Quoted 4 lines

[From your other email]

I have to admit I'm not much of a fan of how you use this apply() callback, is it really necessary?

Why ask, if you already know the answer? :-)

I'm not a big fan either, seemed like the easiest approach at the time. The idea was to keep the hastable (struct hsearch_data) as decoupled as possible from the environment (env_htab which is, in fact, the only instance of struct hsearch_data).

What if the function pointer was stored within the hastable itself? Sort of a virtual method.
This way we get rid of the typedef and the function pointer as a parameter altogether.
The callback parameter then just becomes a boolean value (meaning, do/don't call the callback function stored within the hashtable itself). I like that much better. What do you think?

[...]

Quoted 10 lines

Quoted 2 lines

/* Flags for himport_r() */
#define H_NOCLEAR 1 /* do not clear hash table before

importing */

Quoted 1 line

+#define H_FORCE 2 /* overwrite read-only/write-once

variables */

Make this 1<< x please.

OK.

Quoted 11 lines

Quoted 11 lines

  #endif /* search.h */

lib/hashtable.c +0 -0

index abd61c8..75b9b07 100644
@@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const
 char sep, * himport()
 
   */
 

Quoted 5 lines

lib/hashtable.c ssize_t hexport_r(struct hsearch_data *htab, const · line 609

Quoted 3 lines

lib/hashtable.c +2 -0

+/* Check whether variable name is amongst vars[] */
+static int process_var(const char *name, int nvars, char * const
 vars[])

You mean check_var()?

I mean is_var_in_set_or_is_set_empty().

Sorry, I'm very, very bad at picking function names. Any suggestion?

Quoted 60 lines

Quoted 46 lines

lib/hashtable.c +14 -0

+{
+	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.
*

lib/hashtable.c +0 -0

@@ -639,7 +655,9 @@ ssize_t hexport_r(struct hsearch_data *htab, const

char sep, */

int himport_r(struct hsearch_data *htab,

lib/hashtable.c +5 -1

-	      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)

Quoted 1 line

debug("DELETE ERROR

##############################\n");

Quoted 10 lines

lib/hashtable.c +4 -0

@@ -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;
+
 

Quoted 6 lines

lib/hashtable.c int himport_r(struct hsearch_data *htab, · line 772

Quoted 3 lines

lib/hashtable.c +0 -0

  		/* enter into hash table */
  		e.key = name;
  		e.data = value;

Do you need to do this if you eventually later figure out you have no apply() callback and you did this for no reason?

You mean calling process_var()? It's two separate things.

One, filter out the variables that were not asked to be processed, if there were any (call to process_var())
Two, check whether the new value is acceptable and/or apply it (call apply())
You could have none, either, or both.

Or else, if you mean moving the e.key = name, e.data = value assignments, you're right, they should be moved down (but in that case it would be because the apply function fails, not because it's not present -- default case is always successful).

Quoted 19 lines

Quoted 19 lines

lib/hashtable.c +17 -0

+		/* 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;
+			}
+		}
+
 

Quoted 6 lines

Quoted 4 lines

lib/hashtable.c +0 -0

  		hsearch_r(e, ENTER,&rv, htab);

  		if (rv == NULL) {

printf("himport_r: can't insert \"%s=%s\" into hash

table\n",

Thank you,
Gerlando

Dear Gerlando Falauto,

Quoted 15 lines · Marek Vasut

Quoted 3 lines

Dear Gerlando Falauto,

WD prodded me too long to review this patchset ;-D

Well, better late than never! ;-)

[...]

Quoted 5 lines

common/cmd_nvedit.c int _do_env_set(int flag, int argc, char * const argv[]) · line 335

Quoted 3 lines

common/cmd_nvedit.c +3 -0

+#if defined(CONFIG_CMD_NET)
+	else if (strcmp(name, "bootfile") == 0) {
+		copy_filename(BootFile, newval, sizeof(BootFile));

Can you remove the camel-case here please?

Quoted 1 line

That's code I just moved around... Will do, sir.

Don't call me that way, makes me feel old :D

Quoted 38 lines

Quoted 7 lines

Quoted 7 lines

common/cmd_nvedit.c +6 -0

+		return 0;
+	}
+#endif
+	return 0;
+}
+

[...]

Quoted 16 lines

include/search.h typedef struct entry { · line 54

Quoted 13 lines

include/search.h +1 -0

@@ -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.
+ */

include/search.h +2 -0

+typedef int (*apply_cb)(const char *name, const char *oldval,
+			const char *newval, int flag);

Is the typedef really necessary ?

Quoted 4 lines

[From your other email]

I have to admit I'm not much of a fan of how you use this apply() callback, is it really necessary?

Why ask, if you already know the answer? :-)

I'm not a big fan either, seemed like the easiest approach at the time. The idea was to keep the hastable (struct hsearch_data) as decoupled as possible from the environment (env_htab which is, in fact, the only instance of struct hsearch_data).

Quoted 7 lines

What if the function pointer was stored within the hastable itself? Sort of a virtual method.
This way we get rid of the typedef and the function pointer as a parameter altogether.
The callback parameter then just becomes a boolean value (meaning, do/don't call the callback function stored within the hashtable itself). I like that much better. What do you think?

Don't we always use only one (this callback) function?

Quoted 28 lines

[...]

Quoted 10 lines

Quoted 2 lines

   /* Flags for himport_r() */
   #define	H_NOCLEAR	1	/* do not clear hash table before

importing */

Quoted 1 line

+#define H_FORCE 2 /* overwrite read-only/write-once

variables */

Make this 1<< x please.

OK.

Quoted 11 lines

Quoted 11 lines

   #endif /* search.h */

lib/hashtable.c +0 -0

index abd61c8..75b9b07 100644
@@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab, const
 char sep, * himport()
 
    */
 

Quoted 7 lines

Quoted 5 lines

lib/hashtable.c ssize_t hexport_r(struct hsearch_data *htab, const · line 609

Quoted 3 lines

lib/hashtable.c +2 -0

+/* Check whether variable name is amongst vars[] */
+static int process_var(const char *name, int nvars, char * const
 vars[])

You mean check_var()?

I mean is_var_in_set_or_is_set_empty().

Nice name :)

Quoted 2 lines

Sorry, I'm very, very bad at picking function names. Any suggestion?

The above is quite descriptive ... maybe is_var_in_set() ? And hey, don't be sorry, you're doing very good job!

Quoted 17 lines

Dear Gerlando Falauto,

Quoted 15 lines · Marek Vasut

Quoted 3 lines

Dear Gerlando Falauto,

WD prodded me too long to review this patchset ;-D

Well, better late than never! ;-)

[...]

Quoted 5 lines

common/cmd_nvedit.c int _do_env_set(int flag, int argc, char * const argv[]) · line 335

Quoted 3 lines

common/cmd_nvedit.c +3 -0

+#if defined(CONFIG_CMD_NET)
+	else if (strcmp(name, "bootfile") == 0) {
+		copy_filename(BootFile, newval, sizeof(BootFile));

Can you remove the camel-case here please?

Quoted 3 lines

Quoted 1 line

That's code I just moved around... Will do, sir.

Don't call me that way, makes me feel old :D

Was more of a way to remark authority than age. :-)

Quoted 45 lines

Quoted 44 lines

Quoted 6 lines

Quoted 6 lines

common/cmd_nvedit.c +6 -0

+		return 0;
+	}
+#endif
+	return 0;
+}
+

[...]

Quoted 16 lines

include/search.h typedef struct entry { · line 54

Quoted 13 lines

include/search.h +1 -0

@@ -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. + */

include/search.h +2 -0

+typedef int (*apply_cb)(const char *name, const char *oldval,
+			const char *newval, int flag);

Is the typedef really necessary ?

Quoted 4 lines

[From your other email]

I have to admit I'm not much of a fan of how you use this apply() callback, is it really necessary?

Why ask, if you already know the answer? :-)

I'm not a big fan either, seemed like the easiest approach at the time. The idea was to keep the hastable (struct hsearch_data) as decoupled as possible from the environment (env_htab which is, in fact, the only instance of struct hsearch_data).

What if the function pointer was stored within the hastable itself? Sort of a virtual method.
This way we get rid of the typedef and the function pointer as a parameter altogether.
The callback parameter then just becomes a boolean value (meaning, do/don't call the callback function stored within the hashtable itself). I like that much better. What do you think?

Quoted 1 line

Don't we always use only one (this callback) function?

Yes, but only because env is the only hashtable present. Is that a yes or a no, then?

Quoted 4 lines

Quoted 4 lines

[...]

Quoted 2 lines

Quoted 2 lines

   /* Flags for himport_r() */
   #define	H_NOCLEAR	1	/* do not clear hash table 

Quoted 32 lines

Quoted 22 lines

Quoted 8 lines

importing */

Quoted 1 line

+#define H_FORCE 2 /* overwrite read-only/write-once

variables */

Make this 1<< x please.

OK.

Quoted 11 lines

Quoted 11 lines

   #endif /* search.h */

lib/hashtable.c +0 -0

index abd61c8..75b9b07 100644
@@ -603,6 +603,22 @@ ssize_t hexport_r(struct hsearch_data *htab,
 const char sep, * himport()
 
    */
 

Quoted 7 lines

Quoted 5 lines

lib/hashtable.c ssize_t hexport_r(struct hsearch_data *htab, · line 609

Quoted 3 lines

lib/hashtable.c +2 -0

+/* Check whether variable name is amongst vars[] */
+static int process_var(const char *name, int nvars, char * const
 vars[])

You mean check_var()?

I mean is_var_in_set_or_is_set_empty().

Nice name :)

Quoted 5 lines

Quoted 2 lines

Sorry, I'm very, very bad at picking function names. Any suggestion?

The above is quite descriptive ... maybe is_var_in_set() ? And hey, don't be sorry, you're doing very good job!

I like is_var_in_set().

Thanks,
Gerlando

Dear Gerlando Falauto,

Quoted 4 lines · Marek Vasut

Quoted 3 lines

Dear Gerlando Falauto,

Quoted 1 line · Marek Vasut

Quoted 45 lines

Quoted 45 lines

Quoted 44 lines

Quoted 6 lines

Quoted 6 lines

common/cmd_nvedit.c +6 -0

+		return 0;
+	}
+#endif
+	return 0;
+}
+

[...]

Quoted 16 lines

include/search.h typedef struct entry { · line 54

Quoted 13 lines

include/search.h +1 -0

@@ -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. + */

include/search.h +2 -0

+typedef int (*apply_cb)(const char *name, const char *oldval,
+			const char *newval, int flag);

Is the typedef really necessary ?

Quoted 4 lines

[From your other email]

I have to admit I'm not much of a fan of how you use this apply() callback, is it really necessary?

Why ask, if you already know the answer? :-)

I'm not a big fan either, seemed like the easiest approach at the time. The idea was to keep the hastable (struct hsearch_data) as decoupled as possible from the environment (env_htab which is, in fact, the only instance of struct hsearch_data).

What if the function pointer was stored within the hastable itself? Sort of a virtual method.
This way we get rid of the typedef and the function pointer as a parameter altogether.
The callback parameter then just becomes a boolean value (meaning, do/don't call the callback function stored within the hashtable itself). I like that much better. What do you think?

Quoted 4 lines

Quoted 1 line

Don't we always use only one (this callback) function?

Yes, but only because env is the only hashtable present. Is that a yes or a no, then?

Do we expect any more hashtables in the near future ?

Quoted 6 lines

Dear Gerlando Falauto,

Quoted 4 lines · Marek Vasut

Quoted 3 lines

Dear Gerlando Falauto,

Quoted 1 line · Marek Vasut

[...]

Quoted 50 lines

Quoted 49 lines

Quoted 45 lines

Quoted 37 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+		return 0;
+	}
+#endif
+	return 0;
+}
+

[...]

Quoted 16 lines

include/search.h

Quoted 13 lines

include/search.h +1 -0

@@ -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. + */

include/search.h +2 -0

+typedef int (*apply_cb)(const char *name, const char *oldval,
+			const char *newval, int flag);

Is the typedef really necessary ?

Quoted 4 lines

[From your other email]

I have to admit I'm not much of a fan of how you use this apply() callback, is it really necessary?

Why ask, if you already know the answer? :-)

I'm not a big fan either, seemed like the easiest approach at the time. The idea was to keep the hastable (struct hsearch_data) as decoupled as possible from the environment (env_htab which is, in fact, the only instance of struct hsearch_data).

Quoted 7 lines

What if the function pointer was stored within the hastable itself? Sort of a virtual method.
This way we get rid of the typedef and the function pointer as a parameter altogether.
The callback parameter then just becomes a boolean value (meaning, do/don't call the callback function stored within the hashtable itself). I like that much better. What do you think?

Quoted 1 line

Don't we always use only one (this callback) function?

Yes, but only because env is the only hashtable present. Is that a yes or a no, then?

Quoted 1 line

Do we expect any more hashtables in the near future ?

I don't think so. Anyway I would rather avoid calling functions belonging to the environment domain from the hastable domain directly. For that matter, we have a single "struct hsearch_data" instance in the whole project, but we keep passing it around as an argument to the hashtable functions.

Best,
Gerlando

On 03/29/2012 10:19 PM, Marek Vasut wrote:

Quoted 6 lines

Quoted 3 lines

+#if defined(CONFIG_CMD_NET)
+ else if (strcmp(name, "bootfile") == 0) {
+ copy_filename(BootFile, newval, sizeof(BootFile));

Can you remove the camel-case here please?

That's code I just moved around... Will do, sir.

Quoted 4 lines

Quoted 4 lines

lib/hashtable.c +0 -0

index abd61c8..75b9b07 100644

Quoted 14 lines

Quoted 9 lines

lib/hashtable.c +2 -0

@@ -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");

Quoted 4 lines

lib/hashtable.c +1 -0

@@ -743,10 +763,31 @@ int himport_r(struct hsearch_data *htab,
 *sp++ = '\0'; /* terminate value */
++dp;
 

Quoted 11 lines

lib/hashtable.c int himport_r(struct hsearch_data *htab, · line 772

Quoted 7 lines

lib/hashtable.c +4 -0

+ /* 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;

Do you need to do this if you eventually later figure out you have no apply()
callback and you did this for no reason?

You mean calling process_var()? It's two separate things.

One, filter out the variables that were not asked to be processed, if there were any (call to process_var())
Two, check whether the new value is acceptable and/or apply it (call apply())
You could have none, either, or both.

Or else, if you mean moving the e.key = name, e.data = value assignments, you're right, they should be moved down (but in that case it would be because the apply function fails, not because it's not present -- default case is always successful).

Quoted 22 lines

Quoted 22 lines

lib/hashtable.c +17 -0

+ /* 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

Quoted 1 line

table\n",

Thank you,
Gerlando

Signed-off-by Gerlando Falauto <[email protected]>

---

?common/cmd_nvedit.c | ? ?2 +-
?include/search.h ? ?| ? ?6 ++++--
?lib/hashtable.c ? ? | ? 18 ++++++++++++------

?3 files changed, 17 insertions(+), 9 deletions(-)

include/search.h +0 -0

index 2a59e03..7ad4261 100644
@@ -142,7 +142,8 @@ int hcreate_r(size_t nel, struct hsearch_data *htab)

?* be freed and the local static variable can be marked as not used. ?*/

?{
? ? ? ?int i;

Quoted 8 lines

include/search.h +0 -0

index 2a59e03..7ad4261 100644
@@ -142,7 +142,8 @@ int hcreate_r(size_t nel, struct hsearch_data *htab)
  * be freed and the local static variable can be marked as not used.
  */
 

include/search.h int hcreate_r(size_t nel, struct hsearch_data *htab) · line 146

Quoted 3 lines

include/search.h +2 -1

-void hdestroy_r(struct hsearch_data *htab)
+void hdestroy_r(struct hsearch_data *htab,
+               int(*apply)(const char *, const char *, const char *, int))\

You could just use 'apply_cb apply' for that param I think.

Quoted 2 lines

You could just use 'apply_cb apply' for that param I think.

Absolutely. I introduced the typedef at a later stage and forgot to update it there too. Good catch, thanks!

Dear Gerlando Falauto,

Quoted 5 lines · In message<[email protected]> you wrote:

Quoted 2 lines

You could just use 'apply_cb apply' for that param I think.

Absolutely. I introduced the typedef at a later stage and forgot to update it there too. Good catch, thanks!

Run checkpatch, and mind the CodingStyle section about typedefs!

Quoted 2 lines

Run checkpatch, and mind the CodingStyle section about typedefs!

I did run checkpatch, it didn't say a word about this.

What is your recommendation about typedef'ining a function pointer? I immediately suspected that using a callback function might sound like heresy, so if you have any better suggestion please put it forward before I rework this changeset for the fifth time... please.

I just think having the whole, expanded signature as the type for a function-pointer parameter or local variable would just make things too long and redundant.

Just off the top of my head: perhaps a struct with a single function pointer element might look better than a typedef?