[U-Boot] [PATCH] cmd: nvedit: Add filtering during env import

[Quentin Schulz]

On Tue, Mar 27, 2018 at 03:14:59PM +0200, Quentin Schulz wrote:
> On Tue, Mar 27, 2018 at 01:39:25PM +0100, Alex Kiernan wrote:
> > On Tue, Mar 27, 2018 at 10:28 AM, Quentin Schulz
> > <quentin.schulz at bootlin.com> wrote:
> > > Hi Alex,
> > >
> > > On Tue, Mar 27, 2018 at 08:43:26AM +0000, Alex Kiernan wrote:
> > >> When importing variables allow size to be elided using '-' and then
> > >> support a list of variables which restricts what will be picked during
> > >> the import.
> > >>
> > >> Signed-off-by: Alex Kiernan <alex.kiernan at gmail.com>
> > >
> > > I'm pretty sure it's the same goal as this patch[1] I suggested.
> > 
> > It is, maybe it was your message I was thinking of when I asked the
> > question the other day:
> > 
> > https://lists.denx.de/pipermail/u-boot/2018-March/323687.html
> > 
> > > Could you answer in the thread telling you need it as well so that we
> > > could get it merged or at least reviewed?
> > >
> > 
> > Assuming I've understood your patch correctly, I think I can replicate
> > your use case with this:
> > 
> >   env import ... ${whitelisted_vars}
> > 
> > I've two uses for this right now for this - with different white lists:
> > 
> >   # override defaults from uboot.env
> >   if fatload mmc ${mmcdev} ${loadaddr} uboot.env; then
> >           env import -c ${loadaddr} ${filesize} serial# ethaddr
> >   fi
> > 
> >   # source OSTree deployments
> >   if load ${devtype} ${bootpart} ${loadaddr} /boot/loader/uEnv.txt; then
> >           env import -t ${loadaddr} ${filesize} kernel_image
> > kernel_image2 bootargs bootargs2
> >   fi
> > 
> 
> What I don't like with this approach is that it's going to be very hard
> to read the line if you want to import a lot of variables.
> 
> You could do the same with just a
> 
> setenv whitelisted_vars my_var0 my_var1
> env import -w -t ${loadaddr} ${filesize}
> 
> setenv whitelisted_vars my_var2
> env import -w -t ${loadaddr} ${filesize}
> 
> I find it more readable.
> 

OK, so that was a bad example as you could do the same with your patch
:D

What I'm more concerned about is that after we've merged your patch,
it isn't possible to pass multiple parameters at the end of the command.
There might be (I have no example as of today) some things we want to do
in the future that can't be done an other way than passing a list of
arguments at the end of the command and your patch prevent doing so in
the future.

We can do it now with a single option (-w or whatever letter we want to
use) so I guess it's better to leave the option for cases that really
require to have a list of parameters passed to the command.

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/20180327/f552323d/attachment-0001.sig>