Dear Gerlando Falauto,
> 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 Falauto<gerlando.fala...@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)
> >>
> >> {
> >>
> >> 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,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.
I groked that down later in the series, OK :)
>
> >> + 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 */
> >>
> >> }
> >>
> >> /*
> >>
> >> - * 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 */
> >>
> >> 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);
> >
> > What changed above?
>
> Replaced spaces with a tab, so to obey a previously mentioned formatting
> rule.
Good :)
>
> >> 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.
Ah, good point.
>
> >> 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 */
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
