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.
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@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@keymile.com Reviewed-by: Marek Vasut marex@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