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

[Quentin Schulz]

Hi Wolfgang,

On Fri, Jun 29, 2018 at 02:52:29PM +0200, Wolfgang Denk wrote:
> Dear Quentin,
> 
> In message <20180629121942.lm4qbfmm5ya7fsx2 at 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
-------------- 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/a7b9bc8c/attachment.sig>