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