[U-Boot,6/7] getenv_f() env variable exist w/o needing a buffer

From: James Yang <James.Yang@freescale.com>

getenv_f() searches the environment for a variable name and copies the
value of the variable to a buffer pointed to by one of the function's
parameters.  However, this means that the buffer needs to exist and
needs to be of sufficient length (passed as another parameter to
getenv_f()) to hold the requested variable's value, even if all that is
desired is the mere detection of the existence of the variable itself.

This patch removes the requirement that the buffer needs to exist.  If
the pointer to the buffer is set to NULL and the requested variable is
found, getenv_f() returns 1, else it returns -1.  The buffer length
parameter is ignored if the pointer is set to NULL.  The original
functionality of getenv_f() is retained (return number of bytes copied
if variable is found, -1 if not), other than being able to copy the
variable's value to the address 0.

Signed-off-by: James Yang <James.Yang@freescale.com>
---
 common/cmd_nvedit.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Dear York Sun,

In message <1357323245-12455-6-git-send-email-yorksun@freescale.com> you wrote:
> From: James Yang <James.Yang@freescale.com>
> 
> getenv_f() searches the environment for a variable name and copies the
> value of the variable to a buffer pointed to by one of the function's
> parameters.  However, this means that the buffer needs to exist and
> needs to be of sufficient length (passed as another parameter to
> getenv_f()) to hold the requested variable's value, even if all that is
> desired is the mere detection of the existence of the variable itself.
> 
> This patch removes the requirement that the buffer needs to exist.  If
> the pointer to the buffer is set to NULL and the requested variable is

Hm... this adds a special case and as such increases complexity - and
what is the benefit for you? 

In your code, you use this feature exactly once, which means all you
save is a single buffer on the stack of a function that does not
appear to be critical in terms of stack size.

>  /*
>   * Look up variable from environment for restricted C runtime env.
> + * If the variable is found, return the number of bytes copied.
> + * If buf is NULL, len is ignored, and, if the variable is found, return 1.
> + * If the variable is not found, return -1.

I think your description is not quite correct, and I dislike the
inconsistent behaviour we get though your patch.  So far, this
function returns the length of the variable value, or -1 in case of
errors.  This should not change even if we implement the suggested
feature, i. e. even when passing NULL as buffer pointer the function
should still return the length, and not some unrelated result.

> +		/* found */
> +		if (!buf)
> +			return 1;

I tend to NAK this part.

Best regards,

Wolfgang Denk

Hello Wolfgang,


On Fri, 4 Jan 2013, Wolfgang Denk wrote:

> Dear York Sun,
> 
> In message <1357323245-12455-6-git-send-email-yorksun@freescale.com> you wrote:
> > From: James Yang <James.Yang@freescale.com>
> > 
> > getenv_f() searches the environment for a variable name and copies the
> > value of the variable to a buffer pointed to by one of the function's
> > parameters.  However, this means that the buffer needs to exist and
> > needs to be of sufficient length (passed as another parameter to
> > getenv_f()) to hold the requested variable's value, even if all that is
> > desired is the mere detection of the existence of the variable itself.
> > 
> > This patch removes the requirement that the buffer needs to exist.  If
> > the pointer to the buffer is set to NULL and the requested variable is
> 
> Hm... this adds a special case and as such increases complexity - and
> what is the benefit for you? 

The purpose is to avoid having to allocate memory for getenv_f() to 
work.  While the unmodified getenv_f() does let me do that if I pass 
len=0, it has the side effect of printing a warning message that the 
buffer is too small.  I want to avoid this message from being printed 
as well.


> In your code, you use this feature exactly once, which means all you
> save is a single buffer on the stack of a function that does not
> appear to be critical in terms of stack size.

Part 7 of the patchset runs at a point where memory can only be 
allocated from the stack.  The stack is in cache, so any available RAM 
is precious.  The function that calls getenv_f() calls another 
function, so allocating a buffer with an unmodified getenv_f() would 
require the buffer to persist in the calling function's stack frame 
uselessly.  That buffer is of size CONFIG_SYS_CBSIZE, which is either 
256 or 1024, so I wouldn't call it non-critical.

I suppose I could create another function that only calls the 
unmodified getenv_f() and returns a boolean as to whether or not that 
variable exists so that the buffer gets deallocated as soon as the 
function returns, but it would not avoid the need to have that memory 
to be actually allocated on the stack.  Also, if the compiler inlines 
that function (this can be prevented as well), it would still make 
that memory persistent.

I imagine that with the modified getenv_f(), other pre-relocation 
features could be written to utilize the detection of environment 
variables in a similar fashion.  This patch set by itself should not 
be considered as the sole usage case.

 
> >  /*
> >   * Look up variable from environment for restricted C runtime env.
> > + * If the variable is found, return the number of bytes copied.
> > + * If buf is NULL, len is ignored, and, if the variable is found, return 1.
> > + * If the variable is not found, return -1.
> 
> I think your description is not quite correct, and I dislike the
> inconsistent behaviour we get though your patch.  So far, this
> function returns the length of the variable value, or -1 in case of
> errors.  This should not change even if we implement the suggested
> feature, i. e. even when passing NULL as buffer pointer the function
> should still return the length, and not some unrelated result.

The description was not written to be a top-down procedural 
description.  Maybe reordering like this will make it seem more 
correct?

> > + * If buf is NULL, len is ignored, and, if the variable is found, return 1.
> > + * If the variable is found, return the number of bytes copied.
> > + * If the variable is not found, return -1.


> > +		/* found */
> > +		if (!buf)
> > +			return 1;
> 
> I tend to NAK this part.

Would it be acceptable if it returns 0 instead?  The reason I chose 1 
is because all of the 100+ existing usages of getenv_f() check only 
for return value > 0.  I was trying to make it consistent with all of 
those existing usage cases.


Regards,

--James

Dear James Yang,

In message <alpine.LRH.2.00.1301041608190.3906@ra8135-ec1.am.freescale.net> you wrote:
> 
> > Hm... this adds a special case and as such increases complexity - and
> > what is the benefit for you? 
> 
> The purpose is to avoid having to allocate memory for getenv_f() to 

What exactly is the problem of adding a dynamic variable on the stack?
This is a way cheaper operation than adding the code here...

> work.  While the unmodified getenv_f() does let me do that if I pass 
> len=0, it has the side effect of printing a warning message that the 
> buffer is too small.  I want to avoid this message from being printed 
> as well.

Then just provide a big enough buffer.  You don't bother about a few
bytes of stack space, do you?  They cost you nothing...

> Part 7 of the patchset runs at a point where memory can only be 
> allocated from the stack.  The stack is in cache, so any available RAM 
> is precious.  The function that calls getenv_f() calls another 

This argument backfires - because if you detect that the variable is
set, then you will call fsl_ddr_interactive(), which then will alocate
a buffer (char buffer2[CONFIG_SYS_CBSIZE]) and call getenv_f() again,
now for real.

Actually you now need TWO such buffers - see this snippet from your
patch 7/7:

        unsigned long long ddrsize;
        const char *prompt = "FSL DDR>";
        char buffer[CONFIG_SYS_CBSIZE];
+       char buffer2[CONFIG_SYS_CBSIZE];
+       char *p = NULL;
        char *argv[CONFIG_SYS_MAXARGS + 1];     /* NULL terminated */
        int argc;
        unsigned int next_step = STEP_GET_SPD;

I. e. before one such buffer was sufficient, now you need two - if
you care about memory, then dumpt his patch, and leave the code as is
- both your code and your stack footprint will be smaller.

> function, so allocating a buffer with an unmodified getenv_f() would 
> require the buffer to persist in the calling function's stack frame 
> uselessly.  That buffer is of size CONFIG_SYS_CBSIZE, which is either 
> 256 or 1024, so I wouldn't call it non-critical.

But you do this anyway, just in another part of the code.  ANd there
you even need two such buffers now!

> I imagine that with the modified getenv_f(), other pre-relocation 
> features could be written to utilize the detection of environment 
> variables in a similar fashion.  This patch set by itself should not 
> be considered as the sole usage case.

Well, the use case you present shows that while the idea sounds good
initially, the results tend to be worse than the existing code.

You did not convince me that the addition is a good idea.

> The description was not written to be a top-down procedural 
> description.  Maybe reordering like this will make it seem more 
> correct?

This will not remove the inconsistent behaviour of returning a 1 in
one case, indepoendent of the actual length of the value, and the
length in another case.  And there is no need for such an
inconsistency.

> > > +		if (!buf)
> > > +			return 1;
> > 
> > I tend to NAK this part.
> 
> Would it be acceptable if it returns 0 instead?  The reason I chose 1 
> is because all of the 100+ existing usages of getenv_f() check only 
> for return value > 0.  I was trying to make it consistent with all of 
> those existing usage cases.

Why don't you implement consistent behaviour and always return the
correct length of the variable value, and -1 if the variable does not
exist?

Best regards,

Wolfgang Denk

York Sun wrote:
> From: James Yang <James.Yang@freescale.com>
> 
> getenv_f() searches the environment for a variable name and copies the
> value of the variable to a buffer pointed to by one of the function's
> parameters.  However, this means that the buffer needs to exist and
> needs to be of sufficient length (passed as another parameter to
> getenv_f()) to hold the requested variable's value, even if all that is
> desired is the mere detection of the existence of the variable itself.
> 
> This patch removes the requirement that the buffer needs to exist.  If
> the pointer to the buffer is set to NULL and the requested variable is
> found, getenv_f() returns 1, else it returns -1.  The buffer length
> parameter is ignored if the pointer is set to NULL.  The original
> functionality of getenv_f() is retained (return number of bytes copied
> if variable is found, -1 if not), other than being able to copy the
> variable's value to the address 0.
> 
> Signed-off-by: James Yang <James.Yang@freescale.com>

Acked-by: Timur Tabi <timur@freescale.com>