On Mon, Jul 2, 2018 at 1:21 PM Quentin Schulz <quentin.sch...@bootlin.com> wrote: > > Hi Wolfgang, > > On Mon, Jul 02, 2018 at 01:21:09PM +0200, Wolfgang Denk wrote: > > Dear Quentin, > > > > In message <20180702092548.bciqnfyd7d2hv26x@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.fala...@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.fala...@keymile.com> > > Reviewed-by: Marek Vasut <ma...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot