[U-Boot] [RFC, PATCH v4 03/16] env: extend interfaces to label variable with context

[AKASHI Takahiro]

On Fri, Jul 19, 2019 at 10:09:19AM +0200, Wolfgang Denk wrote:
> Dear Takahiro,
> 
> In message <20190717082525.891-4-takahiro.akashi at linaro.org> you wrote:
> > The following interfaces are extended to allow for accepting an additional
> > argument, env_context.
> > 	env_get() -> env_get_ext()
> > 	env_set() -> env_get_ext()
> 
> Please don't, see previous comments.

NAK again:)

> > Relevant env commands are synced with this change to maintain the semantics
> > of existing U-Boot environment.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> >  cmd/nvedit.c      | 82 ++++++++++++++++++++++++++++++++++-------------
> >  include/exports.h |  3 ++
> >  2 files changed, 62 insertions(+), 23 deletions(-)
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 49d3b5bdf466..cc80ba712767 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -87,7 +87,7 @@ int get_env_id(void)
> >   *
> >   * Returns 0 in case of error, or length of printed string
> >   */
> > -static int env_print(char *name, int flag)
> > +static int env_print_ext(enum env_context ctx, char *name, int flag)
> >  {
> >  	char *res = NULL;
> >  	ssize_t len;
> > @@ -96,6 +96,7 @@ static int env_print(char *name, int flag)
> >  		ENTRY e, *ep;
> >  
> >  		e.key = name;
> > +		e.context = ctx;
> >  		e.data = NULL;
> >  		hsearch_r(e, FIND, &ep, &env_htab, flag);
> >  		if (ep == NULL)
> > -#if defined(CONFIG_CMD_NVEDIT_EFI)
> > -	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> > -		return do_env_print_efi(cmdtp, flag, --argc, ++argv);
> > -#endif
> > +	if (ctx == ENVCTX_UEFI)
> > +		return do_env_print_efi(cmdtp, flag, argc, argv);
> 
> I don't like this.  It doesn't scale.  ENVCTX_UEFI is just one
> context among others; it should not need a "if" here to handle.

Unfortunately, this is necessary.
As you know, we want to allow different implementations of UEFI
variables while we want to use "env" commands in the same way.
do_env_print_efi(), for example, is implemented using *UEFI interfaces*,
neither env_*() nor h*_r().

> > +{
> > +#if defined(CONFIG_CMD_NVEDIT_EFI)
> > +	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> 
> Please use proper argument handling, options can be combined, so the
> 'e' might not be the second character.

I think there are bunch of code like this in U-Boot. No?

> Also, this should probably changed to support a generic "context"
> specific handling, though "-c context" or such, with U-Boot being
> the default.

Yes, but please note that this option, -e, is already merged
in the upstream.

> This again allows to get rid of all these "if"s

I agree, but only when yet another context be introduced.

Thanks,
-Takahiro Akashi

> > -#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI)
> > -	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> > -		return do_env_set_efi(NULL, flag, --argc, ++argv);
> > -#endif
> > +	if (ctx == ENVCTX_UEFI)
> > +		return do_env_set_efi(NULL, flag, argc, argv);
> 
> Ditto here.
> 
> > +#if CONFIG_IS_ENABLED(CMD_NVEDIT_EFI)
> > +	if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'e')
> > +		return _do_env_set(flag, --argc, ++argv, H_INTERACTIVE,
> > +				   ENVCTX_UEFI);
> > +	else
> > +#endif
> 
> And here.
> 
> >  		const char * const _argv[3] = { "setenv", argv[1], NULL };
> >  
> > -		return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE);
> > +		return _do_env_set(0, 2, (char * const *)_argv, H_INTERACTIVE,
> > +				   ENVCTX_UBOOT);
> >  	} else {
> >  		const char * const _argv[4] = { "setenv", argv[1], buffer,
> >  			NULL };
> >  
> > -		return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE);
> > +		return _do_env_set(0, 3, (char * const *)_argv, H_INTERACTIVE,
> > +				   ENVCTX_UBOOT);
> 
> Also here. ENVCTX_UBOOT is not a special context and should not need
> special handling.
> 
> Best regards,
> 
> Wolfgang Denk
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> I've got to get something inside me. Some coffee  or  something.  And
> then the world will somehow be better.
>                                      - Terry Pratchett, _Men at Arms_