From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gerlando Falauto <gerlando.falauto@keymile.com>
Date: Mon, 12 Dec 2011 10:32:40 +0100
Subject: [U-Boot] [PATCH v2 1/3] env: unify logic to check and apply
	changes
In-Reply-To: <CAPnjgZ2pF19NCryPuzyPDp=Sfd3y1VVfx8FLOA5GoF5rvnB==g@mail.gmail.com>
References: <1321634955-5561-1-git-send-email-gerlando.falauto@keymile.com><1323264605-13541-2-git-send-email-gerlando.falauto@keymile.com>
	<CAPnjgZ2pF19NCryPuzyPDp=Sfd3y1VVfx8FLOA5GoF5rvnB==g@mail.gmail.com>
Message-ID: <4EE5CA38.6090807@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 12/07/2011 11:02 PM, Simon Glass wrote:

[...]
>>   /*
>> - * Set a new environment variable,
>> - * or replace or delete an existing one.
>> + * Performs consistency checking before setting, replacing,
>> + * or deleting an environment variable, then (if successful)
>> + * apply the changes to internals so to make them effective.
>> + * Code for this function was taken out of _do_env_set(),
>> + * which now calls it.
>> + * Also called as a callback function by himport_r().
>> + * Returns 0 in case of success, 1 in case of failure.
>> + * When (flag&  H_FORCE) is set, force overwriting of
>> + * write-once variables.
>
> can you word-wrap that to 72 columns perhaps?

Sorry, I am little confused now. What is the maximum line length?

[...]

>> +/* Check whether variable name is amongst vars[] */
>> +static int process_var(const char *name, int nvars, char * const vars[])
>> +{
>> +       int i = 0;
>
> blank line here.

Thanks, I didn't know about this.

> Can part of this function be #ifdefed to reduce code
> size when the feature is not required?

Uhm, I don't think so. This would be a common feature for selectively 
importing & setting back to default. Unless we want to #ifdef both of 
them. Personally, I would #ifdef neither.

[...]
>
> +                       if (!process_var(name, nvars, vars))
> +                               continue;
>
>                         if (hdelete_r(name, htab) == 0)
>                                 debug("DELETE ERROR
> ##############################\n");
>
> perhaps:
>
> if (process_var(name, nvars, vars)&&
>             hdelete_r(name, htab) == 0)
>       debug("DELETE ERROR ##############################\n");


I think it's easier to read it the original way, and it should not make 
any difference as far as code size is concerned.

> himport_r() is getting a bit overloaded,

Actually, I believe it makes no longer sense to have it called "_r", as 
it was the original reference to the function being recursively 
calleable (i.e. reentrant) as opposed to other versions which were not.

> and it's a shame but I can't
> think of an easy way to refactor it to reduce the number of args. In a
> way you are adding a processing option and so you could put the three
> extra args in a structure and pass a pointer to it (or NULL to skip
> this feature). Not sure though...

Can't think of any other way either, except maybe renaming it and 
re-implementing the shorter version as a wrapper around the newly named 
function. I had already done that, but there would be very few places 
where the old syntax would be kept, so it just didn't make much sense.

> Also, for me this patch adds 500 bytes. I wonder if more of the code
> could made optional?

Frankly, I'm surprised to hear this adds that much overhead.
Actually, I can't see this increase in code size as you mention.
What architecture are you referring to?
In my workspace (ppc_6xx) u-boot.bin and a stripped u-boot ELF file are 
surprisingly unchanged in size, even with debug #defined!
Only place I can experience such growth is within unstripped u-boot ELF, 
which I believe shouldn't really matter... or should it?

Best,
Gerlando