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

[Alex Kiernan]

On Mon, Jul 2, 2018 at 1:21 PM Quentin Schulz
<quentin.schulz at bootlin.com> wrote:
>
> Hi Wolfgang,
>
> On Mon, Jul 02, 2018 at 01:21:09PM +0200, Wolfgang Denk wrote:
> > 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.
> >
>
> OK, good to know.
>
> > > 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.
> >

FWIW I found this surprising too. I ended up redesigning my uses to
either save and restore existing values, or to avoid the requirement
for them altogether.

> > > 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.
> >
>
> It's the flag that is used already for saying to himport_r to NOT delete
> the whole hash table.
>
> See: https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L804
>
> The snippet I sent is actually making use of this flag for something
> other than NOT deleting the whole hash table. The two uses for the flag
> are pretty similar though.
>
> > > 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.
> >
>
> Agreed.
>
> > 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.
> >
>
> Well, if today you do himport_r with a list of vars and the flag
> argument is 0, you actually delete the whole environment.
>
> Today, within the env_import command, there's never a list of vars that
> is passed to himport_r as we can only import the whole environment in
> the given RAM address, so the -d option is pretty safe.
>
> Now that I add the list of vars to env_import, with the patch series I
> sent, if I pass -d and a list of vars to import, the whole environment
> is deleted and only the vars in the list of vars are imported in the
> environnement. This is what you fear and I agree this isn't a very nice
> design.
>
> > So such a default is just dangerous, and (IMO) must be avoided.
> >
>
> I think we misunderstood each other on the proposed patch last mail.
>
> Let me recapitulate what is the current behaviour (correct me if I'm
> wrong).
>
> The current behaviour is the following:
>
> 1) himport_r withOUT a list of variables (vars=NULL) and with flag = 0:
>   = delete the current hash table and then import the environnement,
>   the example for this is basically: env import -d 0xaddr
> 2) himport_r withOUT a list of variables (vars=NULL) and with flag =
> H_NOCLEAR:
>   = do NOT delete the current hash table and then import the environnement,
>   the example for this is basically: env import 0xaddr
> 3) himport_r WITH a list of variables (vars!=NULL) and with flag = 0:
>   = delete the current hash table and then import the variables vars
>   from the environnement to be imported,
>   the example for this is basically: env import -d 0xaddr varA varB varC
> 4) himport_r WITH a list of variables (vars!=NULL) and with flag =
> H_NOCLEAR:
>   = do NOT delete the current hash table and then import the variables vars
>   from the environnement to be imported, IF a var A in vars is not
>   present in the environnement to be imported, var A is removed from the
>   current environnement.
>   the example for this is basically: env import 0xaddr varA varB varC
>
> What I suggested is to modify 3 and 4) to the following:
> 3) himport_r WITH a list of variables (vars!=NULL) and with flag = 0:
>   = import the variables vars from the environnement to be imported, IF
>   a var A in vars is not present in the environnement to be imported,
>   var A is removed from the current environnement.
>   the example for this is basically: env import -d 0xaddr varA varB varC
> 4) himport_r WITH a list of variables (vars!=NULL) and with flag =
> H_NOCLEAR:
>   = import the variables vars from the environnement to be imported,
>   the example for this is basically: env import 0xaddr varA varB varC
>
> This is what the proposed snippet in last mail is supposed to do.
>
> Hopefully, I better explained it. Let me know.
>

Certainly an option to leave existing values there and only overwrite
if them if there are new values in the imported env would be very
useful.


--
Alex Kiernan