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

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

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

[...]
>>
>>   /* 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().
Sorry, I'm very, very bad at picking function names.
Any suggestion?

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

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