On Tue, Nov 19, 2019 at 3:01 PM Simon Goldschmidt <simon.k.r.goldschm...@gmail.com> wrote: > > Heinrich Schuchardt <xypron.g...@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.by...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
