[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