[U-Boot] [RFC PATCH v2 6/7] env: Introduce setenv_transient() helper function
Bernhard Nortmann
bernhard.nortmann at web.de
Tue Nov 22 20:35:53 CET 2016
Hi Simon!
Am 19.11.2016 um 14:47 schrieb Simon Glass:
> Hi Bernhard,
>
> On 16 November 2016 at 03:30, Bernhard Nortmann
> <bernhard.nortmann at web.de> wrote:
>> Like setenv(), but automatically marks the entry as "don't export".
>>
>> Signed-off-by: Bernhard Nortmann <bernhard.nortmann at web.de>
>> ---
>>
>> Changes in v2: None
>>
>> cmd/nvedit.c | 21 +++++++++++++++++++++
>> include/common.h | 1 +
>> 2 files changed, 22 insertions(+)
>>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> But see below.
>
> [...]
> I think returning the error right away is better here.
>
> if (rc)
> return rc;
>
> [...]
> return 0;
Makes sense, I'll change that.
> [...]
> Please add a function comment and parameter names.
There are inconsistent coding styles here. getenv_hex() has a documentation
comment in include/common.h, getenv_yesno() only a standard comment. The
inline function setenv_addr() is both documented and implemented there.
setenv() has no comments at all. Other functions have their documentation
(next to their implementation) in cmd/nvedit.c - which is what I usually
prefer over 'inflating' the header, too.
For setenv_transient() I oriented myself at the 'parent' function setenv(),
but added a documentation comment in cmd/nvedit.c. I'm fine with adding
@param and @return descriptions, and parameter names to include/common.h.
If you insist, I could also move the description over there.
Regards, B. Nortmann
More information about the U-Boot
mailing list