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

[Quentin Schulz]

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

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/20180521/3b140572/attachment.sig>