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

[AKASHI Takahiro]

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

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