[U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'

[Simon Goldschmidt]

Am 19.11.2019 um 18:31 schrieb James Byrne:
> Add env_force() to provide an equivalent to 'setenv -f' that can be used
> programmatically.
> 
> Also tighten up the definition of argv in _do_env_set() so that
> 'const char *' pointers are used.
> 
> Signed-off-by: James Byrne <james.byrne at origamienergy.com>

OK, I'm on CC, so I'll give my two cent:

I always thought this code to be messed up a bit: I think it's better 
programming style to have the "string argument parsing" code call real C 
functions with typed arguments. The env code instead does it the other 
way round (here, you add do_programmatic_env_set() that sets up an 
argv[] array to call another function).

I'm not a maintainer for the ENV code, but maybe that should be sorted 
out instead of adding yet more code that goes this way?

Regards,
Simon

> 
> ---
> 
>   cmd/nvedit.c  | 43 +++++++++++++++++++++++++++++--------------
>   include/env.h | 13 +++++++++++++
>   2 files changed, 42 insertions(+), 14 deletions(-)
> 
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 99a3bc57b1..1f363ba9f4 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -221,10 +221,12 @@ DONE:
>    * Set a new environment variable,
>    * or replace or delete an existing one.
>    */
> -static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
> +static int _do_env_set(int flag, int argc, const char * const argv[],
> +		       int env_flag)
>   {
>   	int   i, len;
> -	char  *name, *value, *s;
> +	const char *name;
> +	char *value, *s;
>   	struct env_entry e, *ep;
>   
>   	debug("Initial value for argc=%d\n", argc);
> @@ -235,7 +237,7 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
>   #endif
>   
>   	while (argc > 1 && **(argv + 1) == '-') {
> -		char *arg = *++argv;
> +		const char *arg = *++argv;
>   
>   		--argc;
>   		while (*++arg) {
> @@ -277,7 +279,7 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
>   		return 1;
>   	}
>   	for (i = 2, s = value; i < argc; ++i) {
> -		char *v = argv[i];
> +		const char *v = argv[i];
>   
>   		while ((*s++ = *v++) != '\0')
>   			;
> @@ -299,18 +301,30 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag)
>   	return 0;
>   }
>   
> -int env_set(const char *varname, const char *varvalue)
> +static int do_programmatic_env_set(int argc, const char * const argv[])
>   {
> -	const char * const argv[4] = { "setenv", varname, varvalue, NULL };
> -
>   	/* before import into hashtable */
>   	if (!(gd->flags & GD_FLG_ENV_READY))
>   		return 1;
>   
> -	if (varvalue == NULL || varvalue[0] == '\0')
> -		return _do_env_set(0, 2, (char * const *)argv, H_PROGRAMMATIC);
> -	else
> -		return _do_env_set(0, 3, (char * const *)argv, H_PROGRAMMATIC);
> +	if (!argv[argc - 1] || argv[argc - 1][0] == '\0')
> +		argc--;
> +
> +	return _do_env_set(0, argc, argv, H_PROGRAMMATIC);
> +}
> +
> +int env_set(const char *varname, const char *varvalue)
> +{
> +	const char * const argv[4] = {"setenv", varname, varvalue, NULL};
> +
> +	return do_programmatic_env_set(3, argv);
> +}
> +
> +int env_force(const char *varname, const char *varvalue)
> +{
> +	const char * const argv[5] = {"setenv", "-f", varname, varvalue, NULL};
> +
> +	return do_programmatic_env_set(4, argv);
>   }
>   
>   /**
> @@ -382,7 +396,8 @@ static int do_env_set(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   	if (argc < 2)
>   		return CMD_RET_USAGE;
>   
> -	return _do_env_set(flag, argc, argv, H_INTERACTIVE);
> +	return _do_env_set(flag, argc, (const char * const *)argv,
> +			   H_INTERACTIVE);
>   }
>   
>   /*
> @@ -643,12 +658,12 @@ static int do_env_edit(cmd_tbl_t *cmdtp, int flag, int argc,
>   	if (buffer[0] == '\0') {
>   		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, _argv, H_INTERACTIVE);
>   	} 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, _argv, H_INTERACTIVE);
>   	}
>   }
>   #endif /* CONFIG_CMD_EDITENV */
> diff --git a/include/env.h b/include/env.h
> index b72239f6a5..37bbf1117d 100644
> --- a/include/env.h
> +++ b/include/env.h
> @@ -145,6 +145,19 @@ int env_get_yesno(const char *var);
>    */
>   int env_set(const char *varname, const char *value);
>   
> +/**
> + * env_force() - forcibly set an environment variable
> + *
> + * This sets or deletes the value of an environment variable. It is the same
> + * as env_set(), except that the variable will be forcibly updated/deleted,
> + * even if it has access protection flags set.
> + *
> + * @varname: Variable to adjust
> + * @value: Value to set for the variable, or NULL or "" to delete the variable
> + * @return 0 if OK, 1 on error
> + */
> +int env_force(const char *varname, const char *varvalue);
> +
>   /**
>    * env_get_ulong() - Return an environment variable as an integer value
>    *
>