[U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import

[Alex Kiernan]

On Mon, May 21, 2018 at 1:34 PM Quentin Schulz <quentin.schulz at bootlin.com>
wrote:

> On Mon, May 21, 2018 at 01:26:11PM +0100, Alex Kiernan wrote:
> > On Mon, May 21, 2018 at 1:06 PM Quentin Schulz <
quentin.schulz at bootlin.com>
> > wrote:
> >
> > > Hi Alex,
> >
> > > On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
> > > > On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
> > quentin.schulz at bootlin.com>
> > > > wrote:
> > > >
> > > > > Hi Stephen,
> > > >
> > > > > On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
> > > > > > On 05/18/2018 08:44 AM, Quentin Schulz 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 a `-w` option that asks `env import` to look for the
> > > > > > > `whitelisted_vars` env variable for a space-separated list of
> > > > variables
> > > > > > > that are whitelisted.
> > > > > >
> > > > > > Would it be better for the -w option to accept a variable name
> > rather
> > > > than
> > > > > > hard-coding it as whitelisted_vars? That way, people could
> > import/export
> > > > > > different sets of variables at different times, and also could
> > choose a
> > > > more
> > > > > > use-case-specific variable name than whitelisted_vars in order
to
> > > > describe
> > > > > > why those variables are "whitelisted".
> > > >
> > > > > This has been raised in the previous version of the patch[1] (of
which
> > > > > you weren't in the mail recipients) and a similar patch[2] made by
> > Alex
> > > > > Kiernan (Cc of this patch series). I'd say it's an ongoing
discussion,
> > > > > though I should have mentioned it in the comments of the patch?
> > > >
> > > > > TL;DR:
> > > > > Proposition 1: Have -w only which "hardcodedly" checks for
> > > > > "whitelisting_vars",
> > > > > +: straightforward implementation of the argument parsing,
> > > > > -: implicit declaration of the list: you have to know to set
> > > > >     whitelisted_vars in the environnement,
> > > >
> > > > > Proposition 2: Have -w followed by one string-word which is the
name
> > of
> > > > > the env variable where to look for the list of whitelisted env
> > > > > variables,
> > > > > +: explicit var to check where whitelist is looked for,
> > > > > -: a bit of complexity added to the parsing of the parameters of
the
> > env
> > > > >     import function,
> > > >
> > > > > Proposition 3: Have -w followed by the list of whitelisted env
> > variable,
> > > > > +: explicit list
> > > > > -: the list cannot be separated by comma (valid character for an
env
> > > > >     variable) or a space (would not be able to distinguish the
last
> > > > >     arguments of the commands which are address and size with size
> > being
> > > > >     optional => how to know if size was passed or not?), what char
> > can be
> > > > >     used to separate env variables in the list?
> > > > >     how does it perform with a very long list of whitelisted
> > variables?
> > > >
> > > >
> > > > Two more thoughts, both of which delegate the separator problem to
the
> > > > caller (the second being the one I implemented as it's almost no
code)
> > > >
> > > > - specify multiple -w options each specifying a whitelisted env
variable
> >
> > > You'll hit the maximum number of arguments/length of the command
quickly
> > > with this method. Quicker than with the other propositions.
> >
> > > Moreover, this can make the command painfully long, painful to read
and
> > > thus cumbersome to find the small typo in your command.
> >
> > > > - use the remaining arguments approach and eat all the trailing
> > arguments
> > > > as the names of env vars you import - needs a sentinel value for the
> > size
> > > > argument
> > > >
> >
> > > That can't work I think.
> >
> > > How do you know if the size argument was passed or not? How'd you know
> > > what string is addr, size or the whitelist (if there is even any)?
> >
> > > env import foo1 foo2 foo3 foo4 addr size
> > > env import foo1 foo2 foo3 addr
> > > env import addr size
> > > env import addr
> >
> >
> > That's why you need a sentinel for the 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
> >   *              text data must be '\0' terminated
> >   *      -r:     handle CRLF like LF, that means exported variables with
> >   *              a content which ends with \r won't get imported. Used
> >   *              to import text files created with editors which are
using
> > CRLF
> >   *              for line endings. Only effective in addition to -t.
> >   *      -b:     assume binary format ('\0' separated, "\0\0"
terminated)
> >   *      -c:     assume checksum protected environment format
> >   *      addr:   memory address to read from
> >   *      size:   length of input data; if missing, proper '\0'
> >   *              termination is mandatory. If not required and passing
> >   *              variables to import use '-'
> >   *      var...: List of variable names that get imported. Without
arguments,
> >   *              all variables are imported
> >
> > Which for your examples translates to:
> >
> > env import addr size foo1 foo2 foo3 foo4
> > env import addr - foo1 foo2 foo3
> > env import addr size
> > env import addr
> >

> Ah, I misunderstood the word sentinel then :)

> Would have been an even better idea if we had some consistency between
> env export and env import. For specifying the env export size, we have
> to use the -s argument but it's a positional argument for env export. We
> then have a list of variables to export, so it would make sense to have
> the same for env import I guess?


I agree, but I can't immediately think of a good way of doing that which is
backward compatible.

-- 
Alex Kiernan