Hi Wolfgang,
On Fri, Jun 29, 2018 at 02:52:29PM +0200, Wolfgang Denk wrote:
Dear Quentin,
In message 20180629121942.lm4qbfmm5ya7fsx2@qschulz you wrote:
I suggest that this isNOT the default behaviour. I think most peaople would expect that this command only adds/replaces environment settings, so omitting one value should not cause unexpected behaviour.
It's the default behaviour of himport_r.
Hm.... I don't see what you mean. himport_r imports a set of new name=value pairs (in different formats); however, ther eis 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
However, there's already a flag parameter so maybe I could just add an H_KEEP_VAR flag to keep the current default behaviour of himport_r (which is in a library) but add the feature you requested in the env import cmd, that should work I guess.
There's already a -d option which basically, if I understood correctly, tells himport_r to erase the current env and use only the imported env.
Um... right - only with the -d option h_import_r will erase any variables - without any name list, it process _all_ variables from the import, and erase _any_ other. With a list of specified names, it seems logical to me that only this list will be processed - i. e. only these will be impoted or erased.
That was what I had in mind with consistent behaviour.
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
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?
i.e. something like:
--- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -749,8 +749,11 @@ static int drop_var_from_set(const char *name, int nvars, char * vars[]) * * The "flag" argument can be used to control the behaviour: when the * H_NOCLEAR bit is set, then an existing hash table will kept, i. e. - * new data will be added to an existing hash table; otherwise, old - * data will be discarded and a new hash table will be created. + * new data will be added to an existing hash table; otherwise, if no + * vars are passed, old data will be discarded and a new hash table + * will be created. If vars are passed, passed vars that are not in + * the linear list of "name=value" pairs will be removed from the + * current hash table. * * The separator character for the "name=value" pairs can be selected, * so we both support importing from externally stored environment @@ -801,7 +804,7 @@ int himport_r(struct hsearch_data *htab, if (nvars) memcpy(localvars, vars, sizeof(vars[0]) * nvars);
- if ((flag & H_NOCLEAR) == 0) { + if ((flag & H_NOCLEAR) == 0 && !nvars) { /* Destroy old hash table if one exists */ debug("Destroy Hash Table: %p table = %p\n", htab, htab->table); @@ -933,6 +936,9 @@ int himport_r(struct hsearch_data *htab, debug("INSERT: free(data = %p)\n", data); free(data);
+ if (flag & H_NOCLEAR) + goto end; + /* process variables which were not considered */ for (i = 0; i < nvars; i++) { if (localvars[i] == NULL) @@ -951,6 +957,7 @@ int himport_r(struct hsearch_data *htab, printf("WARNING: '%s' not in imported env, deleting it!\n", localvars[i]); }
+end: debug("INSERT: done\n"); return 1; /* everything OK */ }
[2] https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L804
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.
Let's add a -k (for H_KEEP_VAR) to tell himport_r we want to keep variables that aren't in the imported env but requested to be loaded (or present in the imported env in one of the forms `var` or `var=`).
Does this make sense? Is it the way you see it implemented?
No. I think it should be done as I explained before. Only with "-d" existing variables shall be deleted, if they are not in the imported blob. Then the behaviour is all the same, only the selection of the names is different: without an argument list, it's all names, and with an argument list it's only the names in the list.
This makes sense. Let's agree on the implementation now :)
Thanks, Quentin