[U-Boot] [PATCH v3 6/6] env: delete selected vars not present in imported env

[Marek Vasut]

Dear Gerlando Falauto,

> On 04/02/2012 09:06 PM, Marek Vasut wrote:
> > Dear Gerlando Falauto,
> > 
> >> When variables explicitly specified on the command line are not present
> >> in the imported env, delete them from the running env.
> >> If the variable is also missing from the running env, issue a warning.
> >> 
> >> Signed-off-by: Gerlando Falauto<gerlando.falauto at keymile.com>
> >> ---
> >> 
> >>   lib/hashtable.c |   48
> >>   +++++++++++++++++++++++++++++++++++++++++------- 1 files changed, 41
> >>   insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/lib/hashtable.c b/lib/hashtable.c
> >> index f3f47de..b3d0b64 100644
> >> --- a/lib/hashtable.c
> >> +++ b/lib/hashtable.c
> >> @@ -607,22 +607,32 @@ ssize_t hexport_r(struct hsearch_data *htab, const
> >> char sep, * himport()
> >> 
> >>    */
> >> 
> >> -/* Check whether variable name is amongst vars[] */
> >> -static int is_var_in_set(const char *name, int nvars, char * const
> >> vars[]) +/*
> >> + * Check whether variable 'name' is amongst vars[],
> >> + * and remove all instances by setting the pointer to NULL
> >> + */
> >> +static int is_var_in_set(const char *name, int nvars, char * vars[])
> >> 
> >>   {
> >>   
> >>   	int i = 0;
> >> 
> >> +	int res = 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;
> >> +		if (vars[i] == NULL)
> >> +			continue;
> >> +		/* If we found it, delete all of them */
> >> +		if (!strcmp(name, vars[i])) {
> >> +			vars[i] = NULL;
> >> +			res = 1;
> >> +		}
> >> 
> >>   	}
> >> 
> >> -	debug("Skipping non-listed variable %s\n", name);
> >> +	if (!res)
> >> +		debug("Skipping non-listed variable %s\n", name);
> >> 
> >> -	return 0;
> >> +	return res;
> >> 
> >>   }
> >>   
> >>   /*
> >> 
> >> @@ -662,9 +672,11 @@ 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 do_apply)
> >> +		int nvars, char * const __vars[], int do_apply)
> >> 
> >>   {
> >>   
> >>   	char *data, *sp, *dp, *name, *value;
> >> 
> >> +	char *vars[nvars];
> >> +	int i;
> >> 
> >>   	/* Test for correct arguments.  */
> >>   	if (htab == NULL) {
> >> 
> >> @@ -681,6 +693,10 @@ int himport_r(struct hsearch_data *htab,
> >> 
> >>   	memcpy(data, env, size);
> >>   	dp = data;
> >> 
> >> +	/* make a local copy of the list of variables */
> >> +	if (nvars)
> >> +		memcpy(vars, __vars, sizeof(__vars[0]) * nvars);
> > 
> > Stupid question -- do you need the local copy at all? Why?
> 
> Because I need some way to keep track of what variables have already
> been taken into account, and it seemed the easiest way to do it without
> touching the original array (which is BTW passed as a const, at least
> pointer-wise).
> I should have written that in the commit message but I forgot.
> 
> I'm not particularly fond of it either, but I'd rather do that than
> overwrite the original array. Not that it's needed afterwards by the
> caller...
> Of course the same information (variables "used") could be tracked in
> some other way (e.g. a bitmask array).

Well won't bitfield suffice then?

> I'm not sure about the binary
> code size, but it would just make things much more complicated to
> read... and it's not like this feature (selective importing) is the core
> of the bootloader, I guess.
> 
> We can of course argue whether going through the hassle of deleting a
> variable specified on the command line which is not defined in the
> default/imported env is really the right thing to do (in other words,
> whether the whole patch has the right to exist!), but that's a different
> story. That's why I enqueued it as a separate patch.

Honestly, I'm not in the position to properly argue here because I'm still 
making myself familiar with the env part of uboot ;-)

> 
> >> +
> >> 
> >>   	if ((flag&  H_NOCLEAR) == 0) {
> >>   	
> >>   		/* Destroy old hash table if one exists */
> >>   		debug("Destroy Hash Table: %p table = %p\n", htab,
> >> 
> >> @@ -809,6 +825,24 @@ int himport_r(struct hsearch_data *htab,
> >> 
> >>   	debug("INSERT: free(data = %p)\n", data);
> >>   	free(data);
> >> 
> >> +	/* process variables which were not considered */
> >> +	for (i = 0; i<  nvars; i++) {
> >> +		if (vars[i] == NULL)
> >> +			continue;
> >> +		/*
> >> +		 * All variables which were not deleted from the variable list
> >> +		 * were not present in the imported env
> >> +		 * This could mean two things:
> >> +		 * a) if the variable was present in current env, we delete it
> >> +		 * b) if the variable was not present in current env, we notify
> >> +		 *    it might be a typo
> >> +		 */
> >> +		if (hdelete_r(vars[i], htab, do_apply) == 0)
> >> +			printf("WARNING: '%s' neither in running nor in imported
> > 
> > env!\n",
> > 
> >> vars[i]); +		else
> >> +			printf("WARNING: '%s' not in imported env, deleting it!
> > 
> > \n", vars[i]);
> > 
> >> +	}
> >> +
> >> 
> >>   	debug("INSERT: done\n");
> >>   	return 1;		/* everything OK */
> >>   
> >>   }