From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gerlando Falauto <gerlando.falauto@keymile.com>
Date: Fri, 30 Mar 2012 19:00:25 +0200
Subject: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply
	changes
In-Reply-To: <4F75AE66.1090205@keymile.com>
References: <1321634955-5561-1-git-send-email-gerlando.falauto@keymile.com>
	<1323264605-13541-2-git-send-email-gerlando.falauto@keymile.com>
	<201203292219.02029.marex@denx.de> <4F75AE66.1090205@keymile.com>
Message-ID: <4F75E6A9.4060602@keymile.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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