From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Wed, 20 Nov 2019 09:10:06 +0900 Subject: [U-Boot] [PATCH] env: Provide programmatic equivalent to 'setenv -f' In-Reply-To: <1daad825-0231-661f-b31b-6d280030138d@gmx.de> References: <0102016e84b84c9c-991687db-659e-4ede-b45e-20864255e87d-000000@eu-west-1.amazonses.com> <79415138-b2bc-78a9-d103-5716a6431cff@gmail.com> <1daad825-0231-661f-b31b-6d280030138d@gmx.de> Message-ID: <20191120001004.GX22427@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 > > > >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 > >>   * > >> > > > > >