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

[Quentin Schulz]

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.
> 
> > 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.

Quentin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180702/12cefd12/attachment.sig>