On Mon, Jul 2, 2018 at 1:21 PM Quentin Schulz quentin.schulz@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.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.
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:
- 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