[U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f'
Joe Hershberger
joe.hershberger at ni.com
Tue Nov 19 21:34:21 UTC 2019
On Tue, Nov 19, 2019 at 3:01 PM Simon Goldschmidt
<simon.k.r.goldschmidt at gmail.com> wrote:
>
> Heinrich Schuchardt <xypron.glpk at 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 at origamienergy.com>
> > >
> > > OK, I'm on CC, so I'll give my two cent:
> > >
> > > I always thought this code to be messed up a bit: I think it's better
> > > programming style to have the "string argument parsing" code call real C
> > > functions with typed arguments. The env code instead does it the other
> > > way round (here, you add do_programmatic_env_set() that sets up an
> > > argv[] array to call another function).
> > >
> > > I'm not a maintainer for the ENV code, but maybe that should be sorted
> > > out instead of adding yet more code that goes this way?
> >
> > There is no maintainer for the ENV code. Simon makes a valid point here.
> > By creating a function for setting variables and separating it from
> > parsing arguments you get the function you need for forcing the value of
> > a variable for free.
> >
>
> 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
More information about the U-Boot
mailing list