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

[Wolfgang Denk]

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.

> 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.

> +{
> +#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.

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

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

> -#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_