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

[Marek Vasut]

Dear Gerlando Falauto,

> On 03/29/2012 10:19 PM, Marek Vasut wrote:
> > Dear Gerlando Falauto,
> > 
> > WD prodded me too long to review this patchset ;-D
> 
> Well, better late than never! ;-)
> 
> [...]
> 
> >> +#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?
> 
> That's code I just moved around... Will do, sir.

Don't call me that way, makes me feel old :D

> >> +		return 0;
> >> +	}
> >> +#endif
> >> +	return 0;
> >> +}
> >> +
> 
> [...]
> 
> >> --- 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 ?
> > 
>  >[From your other email]
>  >
>  > I have to admit I'm not much of a fan of how you use this apply()
>  > callback, is it really necessary?
> 
> Why ask, if you already know the answer? :-)
> 
> I'm not a big fan either, seemed like the easiest approach at the time.
> The idea was to keep the hastable (struct hsearch_data) as decoupled as
> possible from the environment (env_htab which is, in fact, the only
> instance of struct hsearch_data).
> 
> What if the function pointer was stored within the hastable itself?
> Sort of a virtual method.
> This way we get rid of the typedef and the function pointer as a
> parameter altogether.
> The callback parameter then just becomes a boolean value (meaning,
> do/don't call the callback function stored within the hashtable itself).
> I like that much better. What do you think?

Don't we always use only one (this callback) function?

> 
> [...]
> 
> >>   /* 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.
> 
> OK.
> 
> >>   #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()?
> 
> I mean is_var_in_set_or_is_set_empty().

Nice name :)

> Sorry, I'm very, very bad at picking function names.
> Any suggestion?

The above is quite descriptive ... maybe is_var_in_set() ? And hey, don't be 
sorry, you're doing very good job!

> 
> >> +{
> >> +	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?
> 
> You mean calling process_var()? It's two separate things.
> 
> One, filter out the variables that were not asked to be processed, if
> there were any (call to process_var())
> Two, check whether the new value is acceptable and/or apply it (call
> apply())
> You could have none, either, or both.
> 
> Or else, if you mean moving the e.key = name, e.data = value
> assignments, you're right, they should be moved down (but in that case
> it would be because the apply function fails, not because it's not
> present -- default case is always successful).

Yep, that's what I meant. OK

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