[U-Boot] env: Provide programmatic equivalent to 'setenv -f'
diff mbox series

Message ID 0102016e84b84c9c-991687db-659e-4ede-b45e-20864255e87d-000000@eu-west-1.amazonses.com
State Superseded
Delegated to: Tom Rini
Headers show
Series
  • [U-Boot] env: Provide programmatic equivalent to 'setenv -f'
Related show

Commit Message

James Byrne Nov. 19, 2019, 5:31 p.m. UTC
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@origamienergy.com>

---

 cmd/nvedit.c  | 43 +++++++++++++++++++++++++++++--------------
 include/env.h | 13 +++++++++++++
 2 files changed, 42 insertions(+), 14 deletions(-)

Comments

Simon Goldschmidt Nov. 19, 2019, 8:30 p.m. UTC | #1
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@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
>    *
>
Heinrich Schuchardt Nov. 19, 2019, 8:56 p.m. UTC | #2
On 11/19/19 9:30 PM, Simon Goldschmidt wrote:
> 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@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?

There is no maintainer for the ENV code. Simon makes a valid point here.
By creating a function for setting variables and separating it from
parsing arguments you get the function you need for forcing the value of
a variable for free.

Best regards

Heinrich

>
> 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
>>    *
>>
>
>
Simon Goldschmidt Nov. 19, 2019, 9:01 p.m. UTC | #3
Heinrich Schuchardt <xypron.glpk@gmx.de> schrieb am Di., 19. Nov. 2019,
21:56:

> On 11/19/19 9:30 PM, Simon Goldschmidt wrote:
> > 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@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?
>
> There is no maintainer for the ENV code. Simon makes a valid point here.
> By creating a function for setting variables and separating it from
> parsing arguments you get the function you need for forcing the value of
> a variable for free.
>

Right. I thought someone had volunteered but a look at the maintainers file
proves me wrong.

In any way, I'd be more open to review a cleanup patch than a patch
continuing this messy code flow.

Regards,
Simon


> Best regards
>
> Heinrich
>
> >
> > 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
> >>    *
> >>
> >
> >
>
>
Joe Hershberger Nov. 19, 2019, 9:33 p.m. UTC | #4
Hi James,

On Tue, Nov 19, 2019 at 11:32 AM James Byrne
<james.byrne@origamienergy.com> wrote:
>
> 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@origamienergy.com>
>
> ---
>
>  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);

Please call it env_force_set()

> +
>  /**
>   * env_get_ulong() - Return an environment variable as an integer value
>   *
> --
> 2.24.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Joe Hershberger Nov. 19, 2019, 9:34 p.m. UTC | #5
On Tue, Nov 19, 2019 at 3:01 PM Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> Heinrich Schuchardt <xypron.glpk@gmx.de> schrieb am Di., 19. Nov. 2019,
> 21:56:
>
> > On 11/19/19 9:30 PM, Simon Goldschmidt wrote:
> > > 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@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?
> >
> > There is no maintainer for the ENV code. Simon makes a valid point here.
> > By creating a function for setting variables and separating it from
> > parsing arguments you get the function you need for forcing the value of
> > a variable for free.
> >
>
> Right. I thought someone had volunteered but a look at the maintainers file
> proves me wrong.

I sent a patch [1] to Tom a while ago, but it hasn't made it in yet.

[1] - https://patchwork.ozlabs.org/patch/1166740/

> In any way, I'd be more open to review a cleanup patch than a patch
> continuing this messy code flow.

I agree that this could be cleaner, but given that it is simple and
following the existing pattern I don't think it needs to be rejected
for not also including a refactoring. I would join you in encouraging
it though.

Cheers,
-Joe

> Regards,
> Simon
>
>
> > Best regards
> >
> > Heinrich
> >
> > >
> > > 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
> > >>    *
> > >>
> > >
> > >
> >
> >
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
AKASHI Takahiro Nov. 20, 2019, 12:10 a.m. UTC | #6
On Tue, Nov 19, 2019 at 09:56:40PM +0100, Heinrich Schuchardt wrote:
> On 11/19/19 9:30 PM, Simon Goldschmidt wrote:
> >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@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?
> 
> There is no maintainer for the ENV code. Simon makes a valid point here.
> By creating a function for setting variables and separating it from
> parsing arguments you get the function you need for forcing the value of
> a variable for free.

+1

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> >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
> >>   *
> >>
> >
> >
>
James Byrne Nov. 20, 2019, 6:49 p.m. UTC | #7
On 19/11/2019 21:01, Simon Goldschmidt wrote:
> 
> 
> Heinrich Schuchardt <xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de>> 
> schrieb am Di., 19. Nov. 2019, 21:56:
> 
>     On 11/19/19 9:30 PM, Simon Goldschmidt wrote:
>      > 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@origamienergy.com
>     <mailto:james.byrne@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?
> 
>     There is no maintainer for the ENV code. Simon makes a valid point here.
>     By creating a function for setting variables and separating it from
>     parsing arguments you get the function you need for forcing the value of
>     a variable for free.
> 
> 
> Right. I thought someone had volunteered but a look at the maintainers 
> file proves me wrong.
> 
> In any way, I'd be more open to review a cleanup patch than a patch 
> continuing this messy code flow.

Having looked at it again, I agree. I have now redone it, but I have 
ended up changing quite a lot more of the underlying code. I will 
resubmit a revised patch (probably tomorrow) in two parts, one to apply 
some tidying up to the env code, and one to add the new function. It 
will be a much bigger patch set though!

James
Simon Goldschmidt Nov. 20, 2019, 8:21 p.m. UTC | #8
Am 20.11.2019 um 19:49 schrieb James Byrne:
> On 19/11/2019 21:01, Simon Goldschmidt wrote:
>>
>>
>> Heinrich Schuchardt <xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de>>
>> schrieb am Di., 19. Nov. 2019, 21:56:
>>
>>      On 11/19/19 9:30 PM, Simon Goldschmidt wrote:
>>       > 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@origamienergy.com
>>      <mailto:james.byrne@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?
>>
>>      There is no maintainer for the ENV code. Simon makes a valid point here.
>>      By creating a function for setting variables and separating it from
>>      parsing arguments you get the function you need for forcing the value of
>>      a variable for free.
>>
>>
>> Right. I thought someone had volunteered but a look at the maintainers
>> file proves me wrong.
>>
>> In any way, I'd be more open to review a cleanup patch than a patch
>> continuing this messy code flow.
> 
> Having looked at it again, I agree. I have now redone it, but I have
> ended up changing quite a lot more of the underlying code. I will
> resubmit a revised patch (probably tomorrow) in two parts, one to apply
> some tidying up to the env code, and one to add the new function. It
> will be a much bigger patch set though!

Cool. I wouldn't want to put this as a burdon on you (meaning to NACK 
this patch like it is), btu a cleanup in that direction would certainly 
be appreciated!

Thanks,
Simon

> 
> James
>
Tom Rini Nov. 20, 2019, 9:24 p.m. UTC | #9
On Tue, Nov 19, 2019 at 09:37:01PM +0000, Joe Hershberger wrote:
> On Tue, Nov 19, 2019 at 3:01 PM Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> wrote:
> >
> > Heinrich Schuchardt <xypron.glpk@gmx.de> schrieb am Di., 19. Nov. 2019,
> > 21:56:
> >
> > > On 11/19/19 9:30 PM, Simon Goldschmidt wrote:
> > > > 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@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?
> > >
> > > There is no maintainer for the ENV code. Simon makes a valid point here.
> > > By creating a function for setting variables and separating it from
> > > parsing arguments you get the function you need for forcing the value of
> > > a variable for free.
> > >
> >
> > Right. I thought someone had volunteered but a look at the maintainers file
> > proves me wrong.
> 
> I sent a patch [1] to Tom a while ago, but it hasn't made it in yet.
> 
> [1] - https://patchwork.ozlabs.org/patch/1166740/

Sorry, I was waiting for an update where you move Wolfgang down to just
'R' for review, and fix the thinko on his last name :)

Patch
diff mbox series

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
  *