diff mbox series

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

Message ID 322e6866c43f4515240ddca9456ee390b6f334c7.1530257385.git-series.quentin.schulz@bootlin.com
State Superseded
Delegated to: Tom Rini
Headers show
Series [U-Boot,v5,1/5] env: add the same prefix to error messages to make it detectable by tests | expand

Commit Message

Quentin Schulz June 29, 2018, 7:41 a.m. UTC
While the `env export` can take as parameters variables to be exported,
`env import` does not have such a mechanism of variable selection.

Let's add the ability to add parameters at the end of the command for
variables to be imported.

Every env variable from the env to be imported passed by parameter to
this command will override the value of the variable in the current env.

If a variable exists in the current env but not in the imported env, if
this variable is passed as a parameter to env import, the variable will
be unset.

If a variable exists in the imported env, the variable in the current
env will be set to the value of the one from the imported env.

All the remaining variables are left untouched.

As the size parameter of env import is positional but optional, let's
add the possibility to use the sentinel '-' for when we don't want to
give the size parameter (when the env is '\0' terminated) but we pass a
list of variables at the end of the command.

env import addr
env import addr -
env import addr size
env import addr - foo1 foo2
env import addr size foo1 foo2

are all valid.

env import -c addr
env import -c addr -
env import -c addr - foo1 foo2

are all invalid because they don't pass the size parameter required for
checking, while the following are valid.

env import addr size
env import addr size foo1 foo2

Nothing's changed for the other parameters or the overall behaviour.

One of its use case could be to load a secure environment from the
signed U-Boot binary and load only a handful of variables from an
other, unsecure, environment without completely losing control of
U-Boot.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
Tested-by: Alex Kiernan <alex.kiernan@gmail.com>
---

v4:
  - add tested-by by Alex,

v3:
  - migrate to env import addr size var... instead of env import -w addr
  size so that the list of variables to load is more explicit and the
  behaviour of env import is closer to the one of env export,

v2:
  - use strdup instead of malloc + strcpy,
  - NULL-check the result of strdup,
  - add common exit path for freeing memory in one unique place,
  - store token pointer from strtok within the char** array instead of
  strdup-ing token within elements of array,


 cmd/nvedit.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Comments

Wolfgang Denk June 29, 2018, 11:03 a.m. UTC | #1
Dear Quentin,

In message <322e6866c43f4515240ddca9456ee390b6f334c7.1530257385.git-series.quentin.schulz@bootlin.com> you wrote:
> While the `env export` can take as parameters variables to be exported,
> `env import` does not have such a mechanism of variable selection.
>
> Let's add the ability to add parameters at the end of the command for
> variables to be imported.

Thanks!

> If a variable exists in the current env but not in the imported env, if
> this variable is passed as a parameter to env import, the variable will
> be unset.

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.

But I agree that such behaviour may also be conveniend - I suggest
that we use an additional option ("-d" ?) to avtiavte it explicitly.

Thanks.

Best regards,

Wolfgang Denk
Quentin Schulz June 29, 2018, 12:19 p.m. UTC | #2
Hi Wolfgang,

On Fri, Jun 29, 2018 at 01:03:25PM +0200, Wolfgang Denk wrote:
> Dear Quentin,
> 
> In message <322e6866c43f4515240ddca9456ee390b6f334c7.1530257385.git-series.quentin.schulz@bootlin.com> you wrote:
> > While the `env export` can take as parameters variables to be exported,
> > `env import` does not have such a mechanism of variable selection.
> >
> > Let's add the ability to add parameters at the end of the command for
> > variables to be imported.
> 
> Thanks!
> 
> > If a variable exists in the current env but not in the imported env, if
> > this variable is passed as a parameter to env import, the variable will
> > be unset.
> 
> 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.

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.

> But I agree that such behaviour may also be conveniend - I suggest
> that we use an additional option ("-d" ?) to avtiavte it explicitly.
> 

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.

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?

Quentin
Wolfgang Denk June 29, 2018, 12:52 p.m. UTC | #3
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.

> 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.

> 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.

Best regards,

Wolfgang Denk
Quentin Schulz July 2, 2018, 9:25 a.m. UTC | #4
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
Wolfgang Denk July 2, 2018, 11:21 a.m. UTC | #5
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
Quentin Schulz July 2, 2018, 12:20 p.m. UTC | #6
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.
> 
> > 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.

Quentin
Alex Kiernan July 2, 2018, 2:59 p.m. UTC | #7
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:
>
> 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
diff mbox series

Patch

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 70d7068..5000517 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -1004,7 +1004,7 @@  sep_err:
 
 #ifdef CONFIG_CMD_IMPORTENV
 /*
- * env import [-d] [-t [-r] | -b | -c] addr [size]
+ * env import [-d] [-t [-r] | -b | -c] addr [size] [var ...]
  *	-d:	delete existing environment before importing;
  *		otherwise overwrite / append to existing definitions
  *	-t:	assume text format; either "size" must be given or the
@@ -1018,6 +1018,11 @@  sep_err:
  *	addr:	memory address to read from
  *	size:	length of input data; if missing, proper '\0'
  *		termination is mandatory
+ *		if var is set and size should be missing (i.e. '\0'
+ *		termination), set size to '-'
+ *	var...	List of the names of the only variables that get imported from
+ *		the environment at address 'addr'. Without arguments, the whole
+ *		environment gets imported.
  */
 static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 			 int argc, char * const argv[])
@@ -1029,6 +1034,7 @@  static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 	int	fmt = 0;
 	int	del = 0;
 	int	crlf_is_lf = 0;
+	int	wl = 0;
 	size_t	size;
 
 	cmd = *argv;
@@ -1077,9 +1083,9 @@  static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 	addr = simple_strtoul(argv[0], NULL, 16);
 	ptr = map_sysmem(addr, 0);
 
-	if (argc == 2) {
+	if (argc >= 2 && strcmp(argv[1], "-")) {
 		size = simple_strtoul(argv[1], NULL, 16);
-	} else if (argc == 1 && chk) {
+	} else if (chk) {
 		puts("## Error: external checksum format must pass size\n");
 		return CMD_RET_FAILURE;
 	} else {
@@ -1101,6 +1107,9 @@  static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 		printf("## Info: input data size = %zu = 0x%zX\n", size, size);
 	}
 
+	if (argc > 2)
+		wl = 1;
+
 	if (chk) {
 		uint32_t crc;
 		env_t *ep = (env_t *)ptr;
@@ -1115,8 +1124,8 @@  static int do_env_import(cmd_tbl_t *cmdtp, int flag,
 		ptr = (char *)ep->data;
 	}
 
-	if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
-			crlf_is_lf, 0, NULL) == 0) {
+	if (!himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
+		       crlf_is_lf, wl ? argc - 2 : 0, wl ? &argv[2] : NULL)) {
 		pr_err("## Error: Environment import failed: errno = %d\n",
 		       errno);
 		return 1;
@@ -1246,7 +1255,7 @@  static char env_help_text[] =
 #endif
 #endif
 #if defined(CONFIG_CMD_IMPORTENV)
-	"env import [-d] [-t [-r] | -b | -c] addr [size] - import environment\n"
+	"env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n"
 #endif
 	"env print [-a | name ...] - print environment\n"
 #if defined(CONFIG_CMD_RUN)