| 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' | expand |
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 > * >
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 >> * >> > >
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 > >> * > >> > > > > > >
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
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
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 > >> * > >> > > > > >
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
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 >
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 :)
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 *
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(-)