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

[Marek Vasut <marex@denx.de>]

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