On 04/02/2012 08:56 PM, Marek Vasut wrote:
Dear Gerlando Falauto,
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.
Signed-off-by: Gerlando Falautogerlando.falauto@keymile.com
common/cmd_nvedit.c | 170 ++++++++++++++++++++++++++++++++------------------- include/search.h | 3 +- 2 files changed, 109 insertions(+), 64 deletions(-)
diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index 22f9821..e762e76 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -197,32 +197,21 @@ static int do_env_grep(cmd_tbl_t *cmdtp, int flag, #endif
/*
- Set a new environment variable,
- or replace or delete an existing one.
- Perform 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 instead.
- Returns 0 in case of success, 1 in case of failure.
- When (flag& H_FORCE) is set, do not print out any error message and
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)
- 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,60 +222,76 @@ 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)Given H_FORCE isn't set on any env var yet, won't this break bisectability?
What do you mean? We are defining (and checking on) a new flag which is not set yet. It's like adding a new feature which nobody uses yet. Of course this patch alone doesn't make any sense on its own, it just sets the ground for features to be used later. But otherwise it should compile (and work) fine, you just can't test it yet.
See, the whole thing started as a single task which kept growing up by adding features which are somehow intertwined. So I tried to break it up into smaller pieces so to at make reviews easier. But logicallly, it's a big fat patch.
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 (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 */ }
/*
* Some variables like "ethaddr" and "serial#" can be set only* once and cannot be deleted; also, "ver" is readonly.
* Some variables like "ethaddr" and "serial#" can be set only 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 */
&& strcmp(ep->data, MK_STR(CONFIG_ETHADDR)) != 0
&& strcmp(oldval, MK_STR(CONFIG_ETHADDR)) != 0- } #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 */*/ if (strcmp(name, "baudrate") == 0) {
- Switch to new baudrate if new baudrate is supported
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);What changed above?
Replaced spaces with a tab, so to obey a previously mentioned formatting rule.
udelay(50000); gd->baudrate = baudrate;#if defined(CONFIG_PPC) || defined(CONFIG_MCF52x2) @@ -300,6 +305,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 ? */
Shouldn't delete-only be handled before env_check_apply() to make it faster?
Why? If you try to unset a variable (e.g. stdin, console), you should *first* check whether that's a sane thing to do, and *then*, that being the case, delete it.
if (argc< 3 || argv[2] == NULL) { int rc = hdelete_r(name,&env_htab); @@ -337,34 +409,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; }
diff --git a/include/search.h b/include/search.h index ef53edb..a4a5ef4 100644 --- a/include/search.h +++ b/include/search.h @@ -99,6 +99,7 @@ extern int himport_r(struct hsearch_data *__htab, int __flag);
/* Flags for himport_r() */ -#define H_NOCLEAR 1 /* do not clear hash table before
importing */
+#define H_NOCLEAR (1<< 0) /* do not clear hash table before
importing */
+#define H_FORCE (1<< 1) /* overwrite read-only/write-once
variables */
#endif /* search.h */