[U-Boot] [PATCH v3 3/6] env: add check/apply logic to himport_r()

[Marek Vasut <marex@denx.de>]

Dear Marek Vasut,

> Dear Gerlando Falauto,
> 
> > Change hashtable so that a callback function will decide whether a
> > variable can be overwritten, and possibly apply the changes.
> > 
> > So add a new field to struct hsearch_data:
> >  o "apply" callback function to check whether a variable can be
> >  
> >     overwritten, and possibly immediately apply the changes;
> >     when NULL, no check is performed.
> > 
> > And a new argument to himport_r():
> >  o "do_apply": whether to call the apply callback function
> > 
> > NOTE: This patch does not change the current behavior.
> > 
> > Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> > ---
> > 
> >  common/cmd_nvedit.c   |    3 ++-
> >  common/env_common.c   |    8 +++++---
> >  include/environment.h |    9 +++++++++
> >  include/search.h      |   14 +++++++++++++-
> >  lib/hashtable.c       |   20 +++++++++++++++++++-
> >  5 files changed, 48 insertions(+), 6 deletions(-)
> > 
> > diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
> > index 59668a6..fc6165f 100644
> > --- a/common/cmd_nvedit.c
> > +++ b/common/cmd_nvedit.c
> > @@ -201,6 +201,7 @@ static int do_env_grep(cmd_tbl_t *cmdtp, int flag,
> > 
> >   * 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.
> > + * 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, do not print out any error message and
> > 
> > force * overwriting of write-once variables.
> > @@ -931,7 +932,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > 
> >  	}
> >  	
> >  	if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR,
> > 
> > -			0, NULL) == 0) {
> > +			0, NULL, 0 /* do_apply */) == 0) {
> > 
> >  		error("Environment import failed: errno = %d\n", errno);
> >  		return 1;
> >  	
> >  	}
> > 
> > diff --git a/common/env_common.c b/common/env_common.c
> > index a93c062..5558c1c 100644
> > --- a/common/env_common.c
> > +++ b/common/env_common.c
> > @@ -122,7 +122,9 @@ const uchar default_environment[] = {
> > 
> >  	"\0"
> >  
> >  };
> > 
> > -struct hsearch_data env_htab;
> > +struct hsearch_data env_htab = {
> > +	.apply = env_check_apply,
> > +};
> > 
> >  static uchar __env_get_char_spec(int index)
> >  {
> > 
> > @@ -183,7 +185,7 @@ void set_default_env(const char *s)
> > 
> >  	if (himport_r(&env_htab, (char *)default_environment,
> >  	
> >  			sizeof(default_environment), '\0', 0,
> > 
> > -			0, NULL) == 0)
> > +			0, NULL, 0 /* do_apply */) == 0)
> 
> Please replace that 0 /*...*/ stuff with simple NULL

Oh, it's integer ... so just remove the comment, sorry.

> 
> >  		error("Environment import failed: errno = %d\n", errno);
> >  	
> >  	gd->flags |= GD_FLG_ENV_READY;
> > 
> > @@ -209,7 +211,7 @@ int env_import(const char *buf, int check)
> > 
> >  	}
> >  	
> >  	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0,
> > 
> > -			0, NULL)) {
> > +			0, NULL, 0 /* do_apply */)) {
> 
> DTTO, otherwise looks fine.
> 
> >  		gd->flags |= GD_FLG_ENV_READY;
> >  		return 1;
> >  	
> >  	}
> > 
> > diff --git a/include/environment.h b/include/environment.h
> > index 1ef44f3..236e179 100644
> > --- a/include/environment.h
> > +++ b/include/environment.h
> > @@ -193,6 +193,15 @@ 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).
> > + * When (flag & H_FORCE) is set, it does not print out any error
> > + * message and forces overwriting of write-once variables.
> > + */
> > +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 94d75fc..721c8ac 100644
> > --- a/include/search.h
> > +++ b/include/search.h
> > @@ -57,6 +57,16 @@ struct hsearch_data {
> > 
> >  	struct _ENTRY *table;
> >  	unsigned int size;
> >  	unsigned int filled;
> > 
> > +/*
> > + * Callback function which will check whether the given change for
> > variable + * "name" from "oldval" to "newval" may be applied or not, and
> > possibly apply + * such change.
> > + * When (flag & H_FORCE) is set, it shall not print out any error
> > message and + * shall force overwriting of write-once variables.
> > +.* Must return 0 for approval, 1 for denial.
> > + */
> > +	int (*apply)(const char *name, const char *oldval,
> > +			const char *newval, int flag);
> > 
> >  };
> >  
> >  /* Create a new hashing table which will at most contain NEL elements. 
> >  */
> > 
> > @@ -97,10 +107,12 @@ extern ssize_t hexport_r(struct hsearch_data
> > *__htab,
> > 
> >  /*
> >  
> >   * nvars: length of vars array
> >   * vars: array of strings (variable names) to import (nvars == 0 means
> > 
> > all) + * do_apply: whether to call callback function to check the new
> > argument, + * and possibly apply changes (false means accept everything)
> > 
> >   */
> >  
> >  extern int himport_r(struct hsearch_data *__htab,
> >  
> >  		     const char *__env, size_t __size, const char __sep,
> > 
> > -		     int __flag, int nvars, char * const vars[]);
> > +		     int __flag, int nvars, char * const vars[], int do_apply);
> > 
> >  /* Flags for himport_r() */
> >  #define	H_NOCLEAR	(1 << 0) /* do not clear hash table before
> 
> importing */
> 
> > diff --git a/lib/hashtable.c b/lib/hashtable.c
> > index 0610e86..6cfba56 100644
> > --- a/lib/hashtable.c
> > +++ b/lib/hashtable.c
> > @@ -658,7 +658,7 @@ static int is_var_in_set(const char *name, int nvars,
> > char * const vars[])
> > 
> >  int himport_r(struct hsearch_data *htab,
> >  
> >  		const char *env, size_t size, const char sep, int flag,
> > 
> > -		int nvars, char * const vars[])
> > +		int nvars, char * const vars[], int do_apply)
> > 
> >  {
> >  
> >  	char *data, *sp, *dp, *name, *value;
> > 
> > @@ -772,6 +772,24 @@ int himport_r(struct hsearch_data *htab,
> > 
> >  		e.key = name;
> >  		e.data = value;
> > 
> > +		/* if there is an apply function, check what it has to say */
> > +		if (do_apply && htab->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 (htab->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",