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 <[email protected]>

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
   *




_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot

Reply via email to