[U-Boot] [PATCH v5 4/5] cmd: nvedit: env import can now import only variables passed as parameters

[Wolfgang Denk]

Dear Quentin,

In message <20180702092548.bciqnfyd7d2hv26x at qschulz> you wrote:
> 
> > Hm.... I don't see what you mean.  himport_r imports a set of new
> > name=value pairs (in different formats); however, there is no way to
> > specify a name without a value, so himport_r by itself will not
> > delete any variables - it only overrides or adds.
> > 
>
> That's not what this comment[1] says. Maybe it's not possible to specify
> a value but there seems to be code to handle this case in himport_r.
>
> [1] https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L881

You are right.  When importing plain text format (-t) then you can
include lines "name=" which will case deletion of this variable.

> When there's a list of vars passed to himport_r, the ones that are not
> found in the imported env are removed from the current environment. This
> is done here:
> https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L936

Again, you are right.  I have to admit that I was not aware of this.
This is not from my original design - it was added later by this
commit d5370febbcbcee3f554df13ed72b7e2b91e5f66c
Author: Gerlando Falauto <gerlando.falauto at keymile.com>
Date:   Sun Aug 26 21:53:00 2012 +0000

    env: delete selected vars not present in imported env

    When variables explicitly specified on the command line are not present
    in the imported env, delete them from the running env.
    If the variable is also missing from the running env, issue a warning.

    Signed-off-by: Gerlando Falauto <gerlando.falauto at keymile.com>
    Reviewed-by: Marek Vasut <marex at denx.de>

In my opinion this is inconsistent, but it seems I missed that patch
when it was posted.  As this happened a looong time ago and noone
since complained, we probably should accept this behaviour as status
quo.

> What about skipping this part if the H_NOCLEAR flag is not set in flags
> and add a condition here[2] to not delete the htable if there's vars
> passed to the function?

Uh... I'm not sure ... How many users will actually understand such
behaviour?  Just reading the sentence "skip this then that flag is
not set" is difficult to parse...

IMO it does not help if the user has to make specific flags
settings.

> The only thing is that we change the behaviour for the people that
> passed variables and NOT H_NOCLEAR. Now the whole hash table won't be
> deleted. There is no user that uses himport_r that way but since the
> code we're discussing is in lib/ I just want to make sure that's fine.

I an really a big fan of the Principle of Least Surprise [1], and my
experience tells me that most users will expect to be able to use
such a command for the purpose to pick a few selected environment
settings from an existing environment blob - say, adjust the nework
settings by picking "ipaddr", "netmask" and "serverip".  And to me
this is also the most useful use case for such a command.

I fear, a large lot of such users will be very badly surprised when
they realize they just blew away all of their other environment.

So such a default is just dangerous, and (IMO) must be avoided.

[1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Perfection is reached, not when there is no longer anything  to  add,
but when there is no longer anything to take away.
                                           - Antoine de Saint-Exupery