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

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

On 03/30/2012 03:00 PM, Gerlando Falauto wrote:
> On 03/29/2012 10:19 PM, Marek Vasut wrote:
[...]
>>> +#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.

Can't do that, sorry. The global symbol "BootFile" has been defined 
somewhere else and it's used all over the place.

[...]
>>> diff --git a/lib/hashtable.c b/lib/hashtable.c
>>> index abd61c8..75b9b07 100644
>>> --- a/lib/hashtable.c
>>> +++ b/lib/hashtable.c
[...]
>>> @@ -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).

Hhmm... sorry, the assignments need to stay where they are.
Values in e.key and e.data are used by hsearch_r() within the if statement.

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

Gerlando