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

[Marek Vasut <marex@denx.de>]

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

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

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

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