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

[Gerlando Falauto <gerlando.falauto@keymile.com>]

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.

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

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

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