diff mbox series

[U-Boot,v3,1/2] cmd: nvedit: env import can now import only variables passed as parameters

Message ID f9a0cd1e36de9c57d74c1599f0835e9b59f56634.1527236883.git-series.quentin.schulz@bootlin.com
State Superseded
Delegated to: Tom Rini
Headers show
Series [U-Boot,v3,1/2] cmd: nvedit: env import can now import only variables passed as parameters | expand

Commit Message

Quentin Schulz May 25, 2018, 8:38 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>
---

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 | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)


base-commit: 8a9dc16e4d07d29fff08b7caca36f0865065f7f7
--
git-series 0.9.1

Comments

Alex Kiernan May 25, 2018, 10:52 a.m. UTC | #1
On Fri, May 25, 2018 at 9:38 AM Quentin Schulz <quentin.schulz@bootlin.com>
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.

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

> 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 | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)

> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 11489b0..18cc4db 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -1001,7 +1001,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
> @@ -1015,6 +1015,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[])
> @@ -1026,6 +1031,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;
> @@ -1074,9 +1080,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 {
> @@ -1098,6 +1104,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;
> @@ -1112,9 +1121,10 @@ 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,

The addition of a '!' seems like an odd change, but looking at what
himport_r() returns, I think you're correct and it's been broken forever.

> +                      crlf_is_lf, wl ? argc - 2 : 0, wl ? &argv[2] :
NULL)) {
>                  pr_err("Environment import failed: errno = %d\n", errno);
> +
>                  return 1;
>          }
>          gd->flags |= GD_FLG_ENV_READY;
> @@ -1242,7 +1252,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)

> base-commit: 8a9dc16e4d07d29fff08b7caca36f0865065f7f7
> --
> git-series 0.9.1

Tested-by: Alex Kiernan <alex.kiernan@gmail.com>
Lothar Waßmann May 25, 2018, 11:26 a.m. UTC | #2
Hi,

On Fri, 25 May 2018 11:52:17 +0100 Alex Kiernan wrote:
> On Fri, May 25, 2018 at 9:38 AM Quentin Schulz <quentin.schulz@bootlin.com>
> 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.
> 
> > 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>
> > ---
> 
> > 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 | 22 ++++++++++++++++------
> >   1 file changed, 16 insertions(+), 6 deletions(-)
> 
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 11489b0..18cc4db 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -1001,7 +1001,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
> > @@ -1015,6 +1015,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[])
> > @@ -1026,6 +1031,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;
> > @@ -1074,9 +1080,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 {
> > @@ -1098,6 +1104,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;
> > @@ -1112,9 +1121,10 @@ 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,
> 
> The addition of a '!' seems like an odd change, but looking at what
> himport_r() returns, I think you're correct and it's been broken forever.
>
No. It was if (h_import_r() == 0) before!
                            ^^^^


Lothar Waßmann
Quentin Schulz May 25, 2018, 12:12 p.m. UTC | #3
Hi Lothar and Alex,

On Fri, May 25, 2018 at 01:26:20PM +0200, Lothar Waßmann wrote:
> Hi,
> 
> On Fri, 25 May 2018 11:52:17 +0100 Alex Kiernan wrote:
> > On Fri, May 25, 2018 at 9:38 AM Quentin Schulz <quentin.schulz@bootlin.com>
> > 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.
> > 
> > > 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>
> > > ---
> > 
> > > 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 | 22 ++++++++++++++++------
> > >   1 file changed, 16 insertions(+), 6 deletions(-)
> > 
> > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > > index 11489b0..18cc4db 100644
> > > --- a/cmd/nvedit.c
> > > +++ b/cmd/nvedit.c
> > > @@ -1001,7 +1001,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
> > > @@ -1015,6 +1015,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[])
> > > @@ -1026,6 +1031,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;
> > > @@ -1074,9 +1080,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 {
> > > @@ -1098,6 +1104,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;
> > > @@ -1112,9 +1121,10 @@ 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,
> > 
> > The addition of a '!' seems like an odd change, but looking at what
> > himport_r() returns, I think you're correct and it's been broken forever.
> >
> No. It was if (h_import_r() == 0) before!
>                             ^^^^

Exactly, with the added ternary conditions, the line exceeds the 80
characters just for the opening bracket. I just replaced the == 0 with a
! to gain a few precious characters and avoid having to do a:

ret = himport_r();
if (!ret)

But I could do it if wanted.

If I had changed the behaviour of the function, I would have made a
separate commit for it so it's not hidden in some unrelated code, so,
that's just a cosmetic fix here :)

Quentin
Alex Kiernan May 25, 2018, 12:17 p.m. UTC | #4
On Fri, May 25, 2018 at 12:26 PM Lothar Waßmann <LW@karo-electronics.de>
wrote:

> Hi,

> On Fri, 25 May 2018 11:52:17 +0100 Alex Kiernan wrote:
> > On Fri, May 25, 2018 at 9:38 AM Quentin Schulz <
quentin.schulz@bootlin.com>
> > 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.
> >
> > > 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>
> > > ---
> >
> > > 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 | 22 ++++++++++++++++------
> > >   1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > > index 11489b0..18cc4db 100644
> > > --- a/cmd/nvedit.c
> > > +++ b/cmd/nvedit.c
> > > @@ -1001,7 +1001,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
> > > @@ -1015,6 +1015,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[])
> > > @@ -1026,6 +1031,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;
> > > @@ -1074,9 +1080,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 {
> > > @@ -1098,6 +1104,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;
> > > @@ -1112,9 +1121,10 @@ 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,
> >
> > The addition of a '!' seems like an odd change, but looking at what
> > himport_r() returns, I think you're correct and it's been broken
forever.
> >
> No. It was if (h_import_r() == 0) before!
>                              ^^^^


I should learn to read properly... thanks and sorry for the noise.
diff mbox series

Patch

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 11489b0..18cc4db 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -1001,7 +1001,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
@@ -1015,6 +1015,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[])
@@ -1026,6 +1031,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;
@@ -1074,9 +1080,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 {
@@ -1098,6 +1104,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;
@@ -1112,9 +1121,10 @@  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("Environment import failed: errno = %d\n", errno);
+
 		return 1;
 	}
 	gd->flags |= GD_FLG_ENV_READY;
@@ -1242,7 +1252,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)