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

[Marek Vasut <marex@denx.de>]

Dear Gerlando Falauto,

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

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

[...]

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

> +		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;
>  	}
> diff --git a/common/env_common.c b/common/env_common.c
> index 8a71096..7e2bb2f 100644
> --- a/common/env_common.c
> +++ b/common/env_common.c
> @@ -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;
>  	}
> diff --git a/include/environment.h b/include/environment.h
> index 3c145af..3a3e6b8 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -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_ */
> 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);

Is the typedef really necessary ?

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

Make this 1 << x please.

> 
>  #endif /* search.h */
> diff --git a/lib/hashtable.c b/lib/hashtable.c
> index abd61c8..75b9b07 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[])

You mean check_var()?

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

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

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