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

[Quentin Schulz]

On Mon, May 21, 2018 at 01:36:49PM +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.
> 
> 
> So you just made me go and read the definition for CONFIG_SYS_MAXARGS, the
> default's clearly pretty small (16). I guess I don't have a feeling for how
> many args you want to import - my use case is for 4, so even with max args
> at
> 16 I'm within that limit.
> 

Same for me, a few variables and that's it. I'm trying not to have a
solution that's working only for me/us with a pretty low limitation
though. Once we've settled for a solution, IMHO, we've to stick with it
and thus if the solution can't be used by others, that's bad design from
us.

I guess at worst, we could call multiple times the env import command
with different whitelists but that's not really user-friendly.

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/4cf6c844/attachment.sig>