* [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
@ 2019-11-19 17:31 James Byrne
2019-11-19 20:30 ` Simon Goldschmidt
2019-11-19 21:33 ` Joe Hershberger
0 siblings, 2 replies; 10+ messages in thread
From: James Byrne @ 2019-11-19 17:31 UTC (permalink / raw)
To: u-boot
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);
+
/**
* env_get_ulong() - Return an environment variable as an integer value
*
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
2019-11-19 17:31 [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f' James Byrne
@ 2019-11-19 20:30 ` Simon Goldschmidt
2019-11-19 20:56 ` Heinrich Schuchardt
2019-11-19 21:33 ` Joe Hershberger
1 sibling, 1 reply; 10+ messages in thread
From: Simon Goldschmidt @ 2019-11-19 20:30 UTC (permalink / raw)
To: u-boot
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
> *
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
2019-11-19 20:30 ` Simon Goldschmidt
@ 2019-11-19 20:56 ` Heinrich Schuchardt
2019-11-19 21:01 ` Simon Goldschmidt
2019-11-20 0:10 ` AKASHI Takahiro
0 siblings, 2 replies; 10+ messages in thread
From: Heinrich Schuchardt @ 2019-11-19 20:56 UTC (permalink / raw)
To: u-boot
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
>> *
>>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
2019-11-19 20:56 ` Heinrich Schuchardt
@ 2019-11-19 21:01 ` Simon Goldschmidt
2019-11-19 21:34 ` Joe Hershberger
2019-11-20 18:49 ` James Byrne
2019-11-20 0:10 ` AKASHI Takahiro
1 sibling, 2 replies; 10+ messages in thread
From: Simon Goldschmidt @ 2019-11-19 21:01 UTC (permalink / raw)
To: u-boot
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
> >> *
> >>
> >
> >
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
2019-11-19 17:31 [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f' James Byrne
2019-11-19 20:30 ` Simon Goldschmidt
@ 2019-11-19 21:33 ` Joe Hershberger
1 sibling, 0 replies; 10+ messages in thread
From: Joe Hershberger @ 2019-11-19 21:33 UTC (permalink / raw)
To: u-boot
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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
2019-11-19 21:01 ` Simon Goldschmidt
@ 2019-11-19 21:34 ` Joe Hershberger
2019-11-20 21:24 ` Tom Rini
2019-11-20 18:49 ` James Byrne
1 sibling, 1 reply; 10+ messages in thread
From: Joe Hershberger @ 2019-11-19 21:34 UTC (permalink / raw)
To: u-boot
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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
2019-11-19 20:56 ` Heinrich Schuchardt
2019-11-19 21:01 ` Simon Goldschmidt
@ 2019-11-20 0:10 ` AKASHI Takahiro
1 sibling, 0 replies; 10+ messages in thread
From: AKASHI Takahiro @ 2019-11-20 0:10 UTC (permalink / raw)
To: u-boot
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
> >> *
> >>
> >
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
2019-11-19 21:01 ` Simon Goldschmidt
2019-11-19 21:34 ` Joe Hershberger
@ 2019-11-20 18:49 ` James Byrne
2019-11-20 20:21 ` Simon Goldschmidt
1 sibling, 1 reply; 10+ messages in thread
From: James Byrne @ 2019-11-20 18:49 UTC (permalink / raw)
To: u-boot
On 19/11/2019 21:01, Simon Goldschmidt wrote:
>
>
> Heinrich Schuchardt <xypron.glpk at 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
2019-11-20 18:49 ` James Byrne
@ 2019-11-20 20:21 ` Simon Goldschmidt
0 siblings, 0 replies; 10+ messages in thread
From: Simon Goldschmidt @ 2019-11-20 20:21 UTC (permalink / raw)
To: u-boot
Am 20.11.2019 um 19:49 schrieb James Byrne:
> On 19/11/2019 21:01, Simon Goldschmidt wrote:
>>
>>
>> Heinrich Schuchardt <xypron.glpk at 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
2019-11-19 21:34 ` Joe Hershberger
@ 2019-11-20 21:24 ` Tom Rini
0 siblings, 0 replies; 10+ messages in thread
From: Tom Rini @ 2019-11-20 21:24 UTC (permalink / raw)
To: u-boot
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 :)
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191120/0492fde9/attachment.sig>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-11-20 21:24 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-19 17:31 [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f' James Byrne
2019-11-19 20:30 ` Simon Goldschmidt
2019-11-19 20:56 ` Heinrich Schuchardt
2019-11-19 21:01 ` Simon Goldschmidt
2019-11-19 21:34 ` Joe Hershberger
2019-11-20 21:24 ` Tom Rini
2019-11-20 18:49 ` James Byrne
2019-11-20 20:21 ` Simon Goldschmidt
2019-11-20 0:10 ` AKASHI Takahiro
2019-11-19 21:33 ` Joe Hershberger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.