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

[Heinrich Schuchardt]

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

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