diff mbox series

[libubootenv] uboot_env: Fix handling of .flags variable when it contains variables which are not created yet

Message ID 3a0d972a-f06b-40ca-8cae-fa52af360afb@googlegroups.com
State New
Headers show
Series [libubootenv] uboot_env: Fix handling of .flags variable when it contains variables which are not created yet | expand

Commit Message

Uwe Schuster Aug. 2, 2019, 11:49 a.m. UTC
This tries to fix the problem with variable access or type flags which are 
part of the .flags variable but do not exist as a env variable as reported 
by me in https://groups.google.com/d/msg/swupdate/ShdD10dl7Pc/ULUYSpFDCAAJ:

1. The write once (o) access atrribute flag is not properly handled if the 
> variable referenced by the .flags value string does not exist yet (as it 
> would be the case with write once variables before they are first written 
> or set). Instead this variables flags gets removed from .flags value and 
> attributes are lost.
> This behavior also applies to all other variables which do not exist yet, 
> but in case of write once variables I'd consider it a normal use case.
>

I had to choose an implementation and I chose to put such uncreated 
variables into the metadata database as normal variables but with NULL 
value assigned.

The patch also fixes a problem with the existing parsing function. It 
failed to handle .flags value strings with more then one variable (comma 
separated).

The only known problem what remains is with fw_printenv function. It does 
currently not print out the .flags variable (as normal u-boot would). To 
fix that more changes to the printenv implementation will be required 
(reconcat .flags from internal list structure) so I decided to omit it.


---
 src/fw_printenv.c |  16 +++--
 src/uboot_env.c   | 176 ++++++++++++++++++++++++++++++----------------
 2 files changed, 125 insertions(+), 67 deletions(-)

         buf += snprintf(buf, size, ".flags=");
@@ -780,10 +789,15 @@ int libuboot_env_store(struct uboot_ctx *ctx)
                         attr_tostring(entry->type),
                         access_tostring(entry->access));
             }
+               first = false;
         }
     }
     *buf++ = '\0';
 
+#if !defined(NDEBUG)
+    fprintf(stdout, "Saved environment FLAGS %s\n", flagsbuf);
+#endif
+
     if (ctx->redundant) {
         unsigned char flags = ctx->envdevs[ctx->current].flags;
         switch(ctx->envdevs[ctx->current].flagstype) {
@@ -817,18 +831,19 @@ int libuboot_env_store(struct uboot_ctx *ctx)
     return ret;
 }
 
-static void libuboot_parseflags(struct uboot_ctx *ctx, char* flagsvar)
+static void libuboot_parseflags(struct uboot_ctx *ctx, const char* flags)
 {
     char *pvar;
     char *pval;
     struct var_entry *entry;
+    char *flagsvar = strdup(flags);
 
 #if !defined(NDEBUG)
-    fprintf(stdout, "Environment FLAGS %s\n", flagsvar);
+    fprintf(stdout, "Parsed environment FLAGS %s\n", flags);
 #endif
     pvar = flagsvar;
 
-    while (*pvar && (pvar - flagsvar) < strlen(flagsvar)) {
+    while (pvar && ((pvar - flagsvar) < strlen(flags))) {
         char *pnext;
         pval = strchr(pvar, ':');
         if (!pval)
@@ -841,47 +856,56 @@ static void libuboot_parseflags(struct uboot_ctx 
*ctx, char* flagsvar)
         else
             *pnext++ = '\0';
 
+#if !defined(NDEBUG)
+        fprintf(stdout, "Parsed var %s:%s\n", pvar, pval);
+#endif
         entry = __libuboot_get_env(&ctx->varlist, pvar);
-        if (entry) {
-            for (int i = 0; i < strlen(pval); i++) {
-                switch (pval[i]) {
-                case 's':
-                    entry->type = TYPE_ATTR_STRING;
-                    break;
-                case 'd':
-                    entry->type = TYPE_ATTR_DECIMAL;
-                    break;
-                case 'x':
-                    entry->type = TYPE_ATTR_HEX;
-                    break;
-                case 'b':
-                    entry->type = TYPE_ATTR_BOOL;
-                    break;
-                case 'i':
-                    entry->type = TYPE_ATTR_IP;
-                    break;
-                case 'm':
-                    entry->type = TYPE_ATTR_MAC;
-                    break;
-                case 'a':
-                    entry->access = ACCESS_ATTR_ANY;
-                    break;
-                case 'r':
-                    entry->access = ACCESS_ATTR_READ_ONLY;
-                    break;
-                case 'o':
-                    entry->access = ACCESS_ATTR_WRITE_ONCE;
-                    break;
-                case 'c':
-                    entry->access = ACCESS_ATTR_CHANGE_DEFAULT;
-                    break;
-                default: /* ignore it */
-                    break;
-                }
+        if (!entry) {
+             /* create non-valued entry to conserve its flags */
+            libuboot_set_env(ctx, pvar, NULL);
+            entry = __libuboot_get_env(&ctx->varlist, pvar);
+        }
+
+        for (int i = 0; i < strlen(pval); i++) {
+            switch (pval[i]) {
+            case 's':
+                entry->type = TYPE_ATTR_STRING;
+                break;
+            case 'd':
+                entry->type = TYPE_ATTR_DECIMAL;
+                break;
+            case 'x':
+                entry->type = TYPE_ATTR_HEX;
+                break;
+            case 'b':
+                entry->type = TYPE_ATTR_BOOL;
+                break;
+            case 'i':
+                entry->type = TYPE_ATTR_IP;
+                break;
+            case 'm':
+                entry->type = TYPE_ATTR_MAC;
+                break;
+            case 'a':
+                entry->access = ACCESS_ATTR_ANY;
+                break;
+            case 'r':
+                entry->access = ACCESS_ATTR_READ_ONLY;
+                break;
+            case 'o':
+                entry->access = ACCESS_ATTR_WRITE_ONCE;
+                break;
+            case 'c':
+                entry->access = ACCESS_ATTR_CHANGE_DEFAULT;
+                break;
+            default: /* ignore it */
+                break;
             }
         }
+
         pvar = pnext;
     }
+    free(flagsvar);
 }
 
 static int libuboot_load(struct uboot_ctx *ctx)
@@ -1061,10 +1085,10 @@ int libuboot_load_file(struct uboot_ctx *ctx, const 
char *filename)
 
         name = buf;
 
-            if (!strcmp(name, ".flags"))
-                flagsvar = strdup(value);
+        if (!strcmp(name, ".flags"))
+            flagsvar = strdup(value);
         else
-                libuboot_set_env(ctx, name, value);
+            libuboot_set_env(ctx, name, value);
     }
 
     /*
@@ -1158,12 +1182,19 @@ static bool libuboot_validate_flags(struct 
var_entry *entry, const char *value)
         ok_access = true;
         break;
     case ACCESS_ATTR_READ_ONLY:
-    case ACCESS_ATTR_WRITE_ONCE:
         ok_access = false;
         break;
+    case ACCESS_ATTR_WRITE_ONCE:
+        if (entry->value) {
+            ok_access = false;
+        }
+        break;
     case ACCESS_ATTR_CHANGE_DEFAULT:
         break;
     }
+#if !defined(NDEBUG)
+    fprintf(stdout, "Access flag validation %s\n", ok_access ? "success." 
: "failed.");
+#endif
 
     if (!ok_access)
         return false;
@@ -1201,6 +1232,10 @@ static bool libuboot_validate_flags(struct var_entry 
*entry, const char *value)
             ok_type = false;
         break;
     }
+#if !defined(NDEBUG)
+    fprintf(stdout, "Type flag validation %s\n", ok_type ? "success." : 
"failed.");
+#endif
+
     return ok_type;
 }
 
@@ -1209,13 +1244,30 @@ int libuboot_set_env(struct uboot_ctx *ctx, const 
char *varname, const char *val
     struct var_entry *entry, *elm, *lastentry;
     struct vars *envs = &ctx->varlist;
 
+    /* update all .flags attributes in list with new value */
+    if (!strcmp(varname, ".flags")) {
+        LIST_FOREACH(entry, &ctx->varlist, next) {
+            entry->type = TYPE_ATTR_STRING;
+            entry->access = ACCESS_ATTR_ANY;
+        }
+        if (value)
+            libuboot_parseflags(ctx, strdup(value));
+
+        return 0;
+     }
+
     entry = __libuboot_get_env(envs, varname);
     if (entry) {
         if (libuboot_validate_flags(entry, value)) {
-            if (!value) {
-                free_var_entry(envs, entry);
-            } else {
-                free(entry->value);
+            if (entry->value) {
+                if (!value) {
+                    free_var_entry(envs, entry);
+                } else {
+                    free(entry->value);
+                    entry->value = strdup(value);
+                }
+            }
+            else if (value) {
                 entry->value = strdup(value);
             }
             return 0;
@@ -1224,9 +1276,6 @@ int libuboot_set_env(struct uboot_ctx *ctx, const 
char *varname, const char *val
         }
     }
 
-    if (!value)
-        return 0;
-
     entry = (struct var_entry *)calloc(1, sizeof(*entry));
     if (!entry)
         return -ENOMEM;
@@ -1235,11 +1284,16 @@ int libuboot_set_env(struct uboot_ctx *ctx, const 
char *varname, const char *val
         free(entry);
         return -ENOMEM;
     }
-    entry->value = strdup(value);
-    if (!entry->value) {
-        free(entry->name);
-        free(entry);
-        return -ENOMEM;
+    if (!value) {
+        entry->value = NULL;
+    }
+     else {
+        entry->value = strdup(value);
+        if (!entry->value) {
+            free(entry->name);
+            free(entry);
+            return -ENOMEM;
+        }
     }
 
     lastentry = NULL;

Comments

Uwe Schuster March 11, 2020, 2:36 p.m. UTC | #1
Hi Stefano,

can this patch be applied ? We are using it in our products since a while, 
but would like to have it part of the community version. Or is there 
something else you need to be prepared?

Uwe

Am Freitag, 2. August 2019 13:49:44 UTC+2 schrieb Uwe Schuster:
>
> This tries to fix the problem with variable access or type flags which are 
> part of the .flags variable but do not exist as a env variable as reported 
> by me in https://groups.google.com/d/msg/swupdate/ShdD10dl7Pc/ULUYSpFDCAAJ
> :
>
> 1. The write once (o) access atrribute flag is not properly handled if the 
>> variable referenced by the .flags value string does not exist yet (as it 
>> would be the case with write once variables before they are first written 
>> or set). Instead this variables flags gets removed from .flags value and 
>> attributes are lost.
>> This behavior also applies to all other variables which do not exist yet, 
>> but in case of write once variables I'd consider it a normal use case.
>>
>
> I had to choose an implementation and I chose to put such uncreated 
> variables into the metadata database as normal variables but with NULL 
> value assigned.
>
> The patch also fixes a problem with the existing parsing function. It 
> failed to handle .flags value strings with more then one variable (comma 
> separated).
>
> The only known problem what remains is with fw_printenv function. It does 
> currently not print out the .flags variable (as normal u-boot would). To 
> fix that more changes to the printenv implementation will be required 
> (reconcat .flags from internal list structure) so I decided to omit it.
>
>
> ---
>  src/fw_printenv.c |  16 +++--
>  src/uboot_env.c   | 176 ++++++++++++++++++++++++++++++----------------
>  2 files changed, 125 insertions(+), 67 deletions(-)
>
> diff --git a/src/fw_printenv.c b/src/fw_printenv.c
> index 18887f9..8fd1f55 100644
> --- a/src/fw_printenv.c
> +++ b/src/fw_printenv.c
> @@ -137,17 +137,21 @@ int main (int argc, char **argv) {
>          if (!argc) {
>              tmp = NULL;
>              while ((tmp = libuboot_iterator(ctx, tmp)) != NULL) {
> -                name = libuboot_getname(tmp);
>                  value = libuboot_getvalue(tmp);
> -                fprintf(stdout, "%s=%s\n", name, value);
> +                if (value) {
> +                    name = libuboot_getname(tmp);
> +                    fprintf(stdout, "%s=%s\n", name, value);
> +                }
>              }
>          } else {
>              for (i = 0; i < argc; i++) {
>                  value = libuboot_get_env(ctx, argv[i]);
> -                if (noheader)
> -                    fprintf(stdout, "%s\n", value ? value : "");
> -                else
> -                    fprintf(stdout, "%s=%s\n", argv[i], value ? value : 
> "");
> +                if (value) {
> +                    if (noheader)
> +                        fprintf(stdout, "%s\n", value ? value : "");
> +                    else
> +                        fprintf(stdout, "%s=%s\n", argv[i], value ? value 
> : "");
> +                }
>              }
>          }
>      } else { /* setenv branch */
> diff --git a/src/uboot_env.c b/src/uboot_env.c
> index 09d210d..bf60635 100644
> --- a/src/uboot_env.c
> +++ b/src/uboot_env.c
> @@ -735,6 +735,9 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>      uint8_t offsetdata;
>      int ret;
>      int copy;
> +#if !defined(NDEBUG)
> +    char * flagsbuf;
> +#endif
>  
>      /*
>       * Allocate the bigger of the case
> @@ -752,21 +755,27 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>  
>      buf = data;
>      LIST_FOREACH(entry, &ctx->varlist, next) {
> -        size = (ctx->size - offsetdata)  - (buf - data); 
> -        if ((strlen(entry->name) + strlen(entry->value) + 2) > size) 
> -            return -ENOMEM;
> +        if (entry->value) {
> +            /* only save variables which have a value (do exist yet) .. */
> +            size = (ctx->size - offsetdata)  - (buf - data); 
> +            if ((strlen(entry->name) + strlen(entry->value) + 2) > size) 
> +                return -ENOMEM;
>  
> +            buf += snprintf(buf, size, "%s=%s", entry->name, 
> entry->value); 
> +            buf++;
> +        }
> +        /* .. but include non yet existing vars into .flags string */
>          if (entry->type || entry->access)
>              saveflags = true;
> -
> -        buf += snprintf(buf, size, "%s=%s", entry->name, entry->value); 
> -        buf++;
>      }
>  
>      /*
>       * Now save the .flags
>       */
>      if (saveflags) {
> +#if !defined(NDEBUG)
> +        flagsbuf = buf;
> +#endif
>          bool first = true;
>          size = (ctx->size - offsetdata)  - (buf - data); 
>          buf += snprintf(buf, size, ".flags=");
> @@ -780,10 +789,15 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>                          attr_tostring(entry->type),
>                          access_tostring(entry->access));
>              }
> +               first = false;
>          }
>      }
>      *buf++ = '\0';
>  
> +#if !defined(NDEBUG)
> +    fprintf(stdout, "Saved environment FLAGS %s\n", flagsbuf);
> +#endif
> +
>      if (ctx->redundant) {
>          unsigned char flags = ctx->envdevs[ctx->current].flags;
>          switch(ctx->envdevs[ctx->current].flagstype) {
> @@ -817,18 +831,19 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>      return ret;
>  }
>  
> -static void libuboot_parseflags(struct uboot_ctx *ctx, char* flagsvar)
> +static void libuboot_parseflags(struct uboot_ctx *ctx, const char* flags)
>  {
>      char *pvar;
>      char *pval;
>      struct var_entry *entry;
> +    char *flagsvar = strdup(flags);
>  
>  #if !defined(NDEBUG)
> -    fprintf(stdout, "Environment FLAGS %s\n", flagsvar);
> +    fprintf(stdout, "Parsed environment FLAGS %s\n", flags);
>  #endif
>      pvar = flagsvar;
>  
> -    while (*pvar && (pvar - flagsvar) < strlen(flagsvar)) {
> +    while (pvar && ((pvar - flagsvar) < strlen(flags))) {
>          char *pnext;
>          pval = strchr(pvar, ':');
>          if (!pval)
> @@ -841,47 +856,56 @@ static void libuboot_parseflags(struct uboot_ctx 
> *ctx, char* flagsvar)
>          else
>              *pnext++ = '\0';
>  
> +#if !defined(NDEBUG)
> +        fprintf(stdout, "Parsed var %s:%s\n", pvar, pval);
> +#endif
>          entry = __libuboot_get_env(&ctx->varlist, pvar);
> -        if (entry) {
> -            for (int i = 0; i < strlen(pval); i++) {
> -                switch (pval[i]) {
> -                case 's':
> -                    entry->type = TYPE_ATTR_STRING;
> -                    break;
> -                case 'd':
> -                    entry->type = TYPE_ATTR_DECIMAL;
> -                    break;
> -                case 'x':
> -                    entry->type = TYPE_ATTR_HEX;
> -                    break;
> -                case 'b':
> -                    entry->type = TYPE_ATTR_BOOL;
> -                    break;
> -                case 'i':
> -                    entry->type = TYPE_ATTR_IP;
> -                    break;
> -                case 'm':
> -                    entry->type = TYPE_ATTR_MAC;
> -                    break;
> -                case 'a':
> -                    entry->access = ACCESS_ATTR_ANY;
> -                    break;
> -                case 'r':
> -                    entry->access = ACCESS_ATTR_READ_ONLY;
> -                    break;
> -                case 'o':
> -                    entry->access = ACCESS_ATTR_WRITE_ONCE;
> -                    break;
> -                case 'c':
> -                    entry->access = ACCESS_ATTR_CHANGE_DEFAULT;
> -                    break;
> -                default: /* ignore it */
> -                    break;
> -                }
> +        if (!entry) {
> +             /* create non-valued entry to conserve its flags */
> +            libuboot_set_env(ctx, pvar, NULL);
> +            entry = __libuboot_get_env(&ctx->varlist, pvar);
> +        }
> +
> +        for (int i = 0; i < strlen(pval); i++) {
> +            switch (pval[i]) {
> +            case 's':
> +                entry->type = TYPE_ATTR_STRING;
> +                break;
> +            case 'd':
> +                entry->type = TYPE_ATTR_DECIMAL;
> +                break;
> +            case 'x':
> +                entry->type = TYPE_ATTR_HEX;
> +                break;
> +            case 'b':
> +                entry->type = TYPE_ATTR_BOOL;
> +                break;
> +            case 'i':
> +                entry->type = TYPE_ATTR_IP;
> +                break;
> +            case 'm':
> +                entry->type = TYPE_ATTR_MAC;
> +                break;
> +            case 'a':
> +                entry->access = ACCESS_ATTR_ANY;
> +                break;
> +            case 'r':
> +                entry->access = ACCESS_ATTR_READ_ONLY;
> +                break;
> +            case 'o':
> +                entry->access = ACCESS_ATTR_WRITE_ONCE;
> +                break;
> +            case 'c':
> +                entry->access = ACCESS_ATTR_CHANGE_DEFAULT;
> +                break;
> +            default: /* ignore it */
> +                break;
>              }
>          }
> +
>          pvar = pnext;
>      }
> +    free(flagsvar);
>  }
>  
>  static int libuboot_load(struct uboot_ctx *ctx)
> @@ -1061,10 +1085,10 @@ int libuboot_load_file(struct uboot_ctx *ctx, 
> const char *filename)
>  
>          name = buf;
>  
> -            if (!strcmp(name, ".flags"))
> -                flagsvar = strdup(value);
> +        if (!strcmp(name, ".flags"))
> +            flagsvar = strdup(value);
>          else
> -                libuboot_set_env(ctx, name, value);
> +            libuboot_set_env(ctx, name, value);
>      }
>  
>      /*
> @@ -1158,12 +1182,19 @@ static bool libuboot_validate_flags(struct 
> var_entry *entry, const char *value)
>          ok_access = true;
>          break;
>      case ACCESS_ATTR_READ_ONLY:
> -    case ACCESS_ATTR_WRITE_ONCE:
>          ok_access = false;
>          break;
> +    case ACCESS_ATTR_WRITE_ONCE:
> +        if (entry->value) {
> +            ok_access = false;
> +        }
> +        break;
>      case ACCESS_ATTR_CHANGE_DEFAULT:
>          break;
>      }
> +#if !defined(NDEBUG)
> +    fprintf(stdout, "Access flag validation %s\n", ok_access ? "success." 
> : "failed.");
> +#endif
>  
>      if (!ok_access)
>          return false;
> @@ -1201,6 +1232,10 @@ static bool libuboot_validate_flags(struct 
> var_entry *entry, const char *value)
>              ok_type = false;
>          break;
>      }
> +#if !defined(NDEBUG)
> +    fprintf(stdout, "Type flag validation %s\n", ok_type ? "success." : 
> "failed.");
> +#endif
> +
>      return ok_type;
>  }
>  
> @@ -1209,13 +1244,30 @@ int libuboot_set_env(struct uboot_ctx *ctx, const 
> char *varname, const char *val
>      struct var_entry *entry, *elm, *lastentry;
>      struct vars *envs = &ctx->varlist;
>  
> +    /* update all .flags attributes in list with new value */
> +    if (!strcmp(varname, ".flags")) {
> +        LIST_FOREACH(entry, &ctx->varlist, next) {
> +            entry->type = TYPE_ATTR_STRING;
> +            entry->access = ACCESS_ATTR_ANY;
> +        }
> +        if (value)
> +            libuboot_parseflags(ctx, strdup(value));
> +
> +        return 0;
> +     }
> +
>      entry = __libuboot_get_env(envs, varname);
>      if (entry) {
>          if (libuboot_validate_flags(entry, value)) {
> -            if (!value) {
> -                free_var_entry(envs, entry);
> -            } else {
> -                free(entry->value);
> +            if (entry->value) {
> +                if (!value) {
> +                    free_var_entry(envs, entry);
> +                } else {
> +                    free(entry->value);
> +                    entry->value = strdup(value);
> +                }
> +            }
> +            else if (value) {
>                  entry->value = strdup(value);
>              }
>              return 0;
> @@ -1224,9 +1276,6 @@ int libuboot_set_env(struct uboot_ctx *ctx, const 
> char *varname, const char *val
>          }
>      }
>  
> -    if (!value)
> -        return 0;
> -
>      entry = (struct var_entry *)calloc(1, sizeof(*entry));
>      if (!entry)
>          return -ENOMEM;
> @@ -1235,11 +1284,16 @@ int libuboot_set_env(struct uboot_ctx *ctx, const 
> char *varname, const char *val
>          free(entry);
>          return -ENOMEM;
>      }
> -    entry->value = strdup(value);
> -    if (!entry->value) {
> -        free(entry->name);
> -        free(entry);
> -        return -ENOMEM;
> +    if (!value) {
> +        entry->value = NULL;
> +    }
> +     else {
> +        entry->value = strdup(value);
> +        if (!entry->value) {
> +            free(entry->name);
> +            free(entry);
> +            return -ENOMEM;
> +        }
>      }
>  
>      lastentry = NULL;
> -- 
> 2.17.1
>
>
>
Stefano Babic March 11, 2020, 4 p.m. UTC | #2
Hi Uwe,

On 11.03.20 15:36, Uwe Schuster wrote:
> Hi Stefano,
> 
> can this patch be applied ?

No.

> We are using it in our products since a
> while, but would like to have it part of the community version. Or is
> there something else you need to be prepared?
>

A patch to be considered must follow some rules. Please check
http://sbabic.github.io/swupdate/contributing.html.

You should add your Signed-off-by to your patches and send them as plain
text, so that it can automatically listed on
http://patchwork.ozlabs.org/project/swupdate/list/, and then applied. It
is like you have copy&paste the code, and this does not allow to apply
the patch without rewriting the code. Use git "format-patch" and "git
send-email" - this guarantees that the mail client won't modify them.

Best regards,
Stefano Babic


> Uwe
> 
> Am Freitag, 2. August 2019 13:49:44 UTC+2 schrieb Uwe Schuster:
> 
>     This tries to fix the problem with variable access or type flags
>     which are part of the .flags variable but do not exist as a env
>     variable as reported by me in
>     https://groups.google.com/d/msg/swupdate/ShdD10dl7Pc/ULUYSpFDCAAJ
>     <https://groups.google.com/d/msg/swupdate/ShdD10dl7Pc/ULUYSpFDCAAJ>:
> 
>         1. The write once (o) access atrribute flag is not properly
>         handled if the variable referenced by the .flags value string
>         does not exist yet (as it would be the case with write once
>         variables before they are first written or set). Instead this
>         variables flags gets removed from .flags value and attributes
>         are lost.
>         This behavior also applies to all other variables which do not
>         exist yet, but in case of write once variables I'd consider it a
>         normal use case.
> 
> 
>     I had to choose an implementation and I chose to put such uncreated
>     variables into the metadata database as normal variables but with
>     NULL value assigned.
> 
>     The patch also fixes a problem with the existing parsing function.
>     It failed to handle .flags value strings with more then one variable
>     (comma separated).
> 
>     The only known problem what remains is with fw_printenv function. It
>     does currently not print out the .flags variable (as normal u-boot
>     would). To fix that more changes to the printenv implementation will
>     be required (reconcat .flags from internal list structure) so I
>     decided to omit it.
> 
> 
>     ---
>      src/fw_printenv.c |  16 +++--
>      src/uboot_env.c   | 176 ++++++++++++++++++++++++++++++----------------
>      2 files changed, 125 insertions(+), 67 deletions(-)
> 
>     diff --git a/src/fw_printenv.c b/src/fw_printenv.c
>     index 18887f9..8fd1f55 100644
>     --- a/src/fw_printenv.c
>     +++ b/src/fw_printenv.c
>     @@ -137,17 +137,21 @@ int main (int argc, char **argv) {
>              if (!argc) {
>                  tmp = NULL;
>                  while ((tmp = libuboot_iterator(ctx, tmp)) != NULL) {
>     -                name = libuboot_getname(tmp);
>                      value = libuboot_getvalue(tmp);
>     -                fprintf(stdout, "%s=%s\n", name, value);
>     +                if (value) {
>     +                    name = libuboot_getname(tmp);
>     +                    fprintf(stdout, "%s=%s\n", name, value);
>     +                }
>                  }
>              } else {
>                  for (i = 0; i < argc; i++) {
>                      value = libuboot_get_env(ctx, argv[i]);
>     -                if (noheader)
>     -                    fprintf(stdout, "%s\n", value ? value : "");
>     -                else
>     -                    fprintf(stdout, "%s=%s\n", argv[i], value ?
>     value : "");
>     +                if (value) {
>     +                    if (noheader)
>     +                        fprintf(stdout, "%s\n", value ? value : "");
>     +                    else
>     +                        fprintf(stdout, "%s=%s\n", argv[i], value ?
>     value : "");
>     +                }
>                  }
>              }
>          } else { /* setenv branch */
>     diff --git a/src/uboot_env.c b/src/uboot_env.c
>     index 09d210d..bf60635 100644
>     --- a/src/uboot_env.c
>     +++ b/src/uboot_env.c
>     @@ -735,6 +735,9 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>          uint8_t offsetdata;
>          int ret;
>          int copy;
>     +#if !defined(NDEBUG)
>     +    char * flagsbuf;
>     +#endif
>      
>          /*
>           * Allocate the bigger of the case
>     @@ -752,21 +755,27 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>      
>          buf = data;
>          LIST_FOREACH(entry, &ctx->varlist, next) {
>     -        size = (ctx->size - offsetdata)  - (buf - data);
>     -        if ((strlen(entry->name) + strlen(entry->value) + 2) > size)
>     -            return -ENOMEM;
>     +        if (entry->value) {
>     +            /* only save variables which have a value (do exist
>     yet) .. */
>     +            size = (ctx->size - offsetdata)  - (buf - data);
>     +            if ((strlen(entry->name) + strlen(entry->value) + 2) >
>     size)
>     +                return -ENOMEM;
>      
>     +            buf += snprintf(buf, size, "%s=%s", entry->name,
>     entry->value);
>     +            buf++;
>     +        }
>     +        /* .. but include non yet existing vars into .flags string */
>              if (entry->type || entry->access)
>                  saveflags = true;
>     -
>     -        buf += snprintf(buf, size, "%s=%s", entry->name,
>     entry->value);
>     -        buf++;
>          }
>      
>          /*
>           * Now save the .flags
>           */
>          if (saveflags) {
>     +#if !defined(NDEBUG)
>     +        flagsbuf = buf;
>     +#endif
>              bool first = true;
>              size = (ctx->size - offsetdata)  - (buf - data);
>              buf += snprintf(buf, size, ".flags=");
>     @@ -780,10 +789,15 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>                              attr_tostring(entry->type),
>                              access_tostring(entry->access));
>                  }
>     +               first = false;
>              }
>          }
>          *buf++ = '\0';
>      
>     +#if !defined(NDEBUG)
>     +    fprintf(stdout, "Saved environment FLAGS %s\n", flagsbuf);
>     +#endif
>     +
>          if (ctx->redundant) {
>              unsigned char flags = ctx->envdevs[ctx->current].flags;
>              switch(ctx->envdevs[ctx->current].flagstype) {
>     @@ -817,18 +831,19 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>          return ret;
>      }
>      
>     -static void libuboot_parseflags(struct uboot_ctx *ctx, char* flagsvar)
>     +static void libuboot_parseflags(struct uboot_ctx *ctx, const char*
>     flags)
>      {
>          char *pvar;
>          char *pval;
>          struct var_entry *entry;
>     +    char *flagsvar = strdup(flags);
>      
>      #if !defined(NDEBUG)
>     -    fprintf(stdout, "Environment FLAGS %s\n", flagsvar);
>     +    fprintf(stdout, "Parsed environment FLAGS %s\n", flags);
>      #endif
>          pvar = flagsvar;
>      
>     -    while (*pvar && (pvar - flagsvar) < strlen(flagsvar)) {
>     +    while (pvar && ((pvar - flagsvar) < strlen(flags))) {
>              char *pnext;
>              pval = strchr(pvar, ':');
>              if (!pval)
>     @@ -841,47 +856,56 @@ static void libuboot_parseflags(struct
>     uboot_ctx *ctx, char* flagsvar)
>              else
>                  *pnext++ = '\0';
>      
>     +#if !defined(NDEBUG)
>     +        fprintf(stdout, "Parsed var %s:%s\n", pvar, pval);
>     +#endif
>              entry = __libuboot_get_env(&ctx->varlist, pvar);
>     -        if (entry) {
>     -            for (int i = 0; i < strlen(pval); i++) {
>     -                switch (pval[i]) {
>     -                case 's':
>     -                    entry->type = TYPE_ATTR_STRING;
>     -                    break;
>     -                case 'd':
>     -                    entry->type = TYPE_ATTR_DECIMAL;
>     -                    break;
>     -                case 'x':
>     -                    entry->type = TYPE_ATTR_HEX;
>     -                    break;
>     -                case 'b':
>     -                    entry->type = TYPE_ATTR_BOOL;
>     -                    break;
>     -                case 'i':
>     -                    entry->type = TYPE_ATTR_IP;
>     -                    break;
>     -                case 'm':
>     -                    entry->type = TYPE_ATTR_MAC;
>     -                    break;
>     -                case 'a':
>     -                    entry->access = ACCESS_ATTR_ANY;
>     -                    break;
>     -                case 'r':
>     -                    entry->access = ACCESS_ATTR_READ_ONLY;
>     -                    break;
>     -                case 'o':
>     -                    entry->access = ACCESS_ATTR_WRITE_ONCE;
>     -                    break;
>     -                case 'c':
>     -                    entry->access = ACCESS_ATTR_CHANGE_DEFAULT;
>     -                    break;
>     -                default: /* ignore it */
>     -                    break;
>     -                }
>     +        if (!entry) {
>     +             /* create non-valued entry to conserve its flags */
>     +            libuboot_set_env(ctx, pvar, NULL);
>     +            entry = __libuboot_get_env(&ctx->varlist, pvar);
>     +        }
>     +
>     +        for (int i = 0; i < strlen(pval); i++) {
>     +            switch (pval[i]) {
>     +            case 's':
>     +                entry->type = TYPE_ATTR_STRING;
>     +                break;
>     +            case 'd':
>     +                entry->type = TYPE_ATTR_DECIMAL;
>     +                break;
>     +            case 'x':
>     +                entry->type = TYPE_ATTR_HEX;
>     +                break;
>     +            case 'b':
>     +                entry->type = TYPE_ATTR_BOOL;
>     +                break;
>     +            case 'i':
>     +                entry->type = TYPE_ATTR_IP;
>     +                break;
>     +            case 'm':
>     +                entry->type = TYPE_ATTR_MAC;
>     +                break;
>     +            case 'a':
>     +                entry->access = ACCESS_ATTR_ANY;
>     +                break;
>     +            case 'r':
>     +                entry->access = ACCESS_ATTR_READ_ONLY;
>     +                break;
>     +            case 'o':
>     +                entry->access = ACCESS_ATTR_WRITE_ONCE;
>     +                break;
>     +            case 'c':
>     +                entry->access = ACCESS_ATTR_CHANGE_DEFAULT;
>     +                break;
>     +            default: /* ignore it */
>     +                break;
>                  }
>              }
>     +
>              pvar = pnext;
>          }
>     +    free(flagsvar);
>      }
>      
>      static int libuboot_load(struct uboot_ctx *ctx)
>     @@ -1061,10 +1085,10 @@ int libuboot_load_file(struct uboot_ctx
>     *ctx, const char *filename)
>      
>              name = buf;
>      
>     -            if (!strcmp(name, ".flags"))
>     -                flagsvar = strdup(value);
>     +        if (!strcmp(name, ".flags"))
>     +            flagsvar = strdup(value);
>              else
>     -                libuboot_set_env(ctx, name, value);
>     +            libuboot_set_env(ctx, name, value);
>          }
>      
>          /*
>     @@ -1158,12 +1182,19 @@ static bool libuboot_validate_flags(struct
>     var_entry *entry, const char *value)
>              ok_access = true;
>              break;
>          case ACCESS_ATTR_READ_ONLY:
>     -    case ACCESS_ATTR_WRITE_ONCE:
>              ok_access = false;
>              break;
>     +    case ACCESS_ATTR_WRITE_ONCE:
>     +        if (entry->value) {
>     +            ok_access = false;
>     +        }
>     +        break;
>          case ACCESS_ATTR_CHANGE_DEFAULT:
>              break;
>          }
>     +#if !defined(NDEBUG)
>     +    fprintf(stdout, "Access flag validation %s\n", ok_access ?
>     "success." : "failed.");
>     +#endif
>      
>          if (!ok_access)
>              return false;
>     @@ -1201,6 +1232,10 @@ static bool libuboot_validate_flags(struct
>     var_entry *entry, const char *value)
>                  ok_type = false;
>              break;
>          }
>     +#if !defined(NDEBUG)
>     +    fprintf(stdout, "Type flag validation %s\n", ok_type ?
>     "success." : "failed.");
>     +#endif
>     +
>          return ok_type;
>      }
>      
>     @@ -1209,13 +1244,30 @@ int libuboot_set_env(struct uboot_ctx *ctx,
>     const char *varname, const char *val
>          struct var_entry *entry, *elm, *lastentry;
>          struct vars *envs = &ctx->varlist;
>      
>     +    /* update all .flags attributes in list with new value */
>     +    if (!strcmp(varname, ".flags")) {
>     +        LIST_FOREACH(entry, &ctx->varlist, next) {
>     +            entry->type = TYPE_ATTR_STRING;
>     +            entry->access = ACCESS_ATTR_ANY;
>     +        }
>     +        if (value)
>     +            libuboot_parseflags(ctx, strdup(value));
>     +
>     +        return 0;
>     +     }
>     +
>          entry = __libuboot_get_env(envs, varname);
>          if (entry) {
>              if (libuboot_validate_flags(entry, value)) {
>     -            if (!value) {
>     -                free_var_entry(envs, entry);
>     -            } else {
>     -                free(entry->value);
>     +            if (entry->value) {
>     +                if (!value) {
>     +                    free_var_entry(envs, entry);
>     +                } else {
>     +                    free(entry->value);
>     +                    entry->value = strdup(value);
>     +                }
>     +            }
>     +            else if (value) {
>                      entry->value = strdup(value);
>                  }
>                  return 0;
>     @@ -1224,9 +1276,6 @@ int libuboot_set_env(struct uboot_ctx *ctx,
>     const char *varname, const char *val
>              }
>          }
>      
>     -    if (!value)
>     -        return 0;
>     -
>          entry = (struct var_entry *)calloc(1, sizeof(*entry));
>          if (!entry)
>              return -ENOMEM;
>     @@ -1235,11 +1284,16 @@ int libuboot_set_env(struct uboot_ctx *ctx,
>     const char *varname, const char *val
>              free(entry);
>              return -ENOMEM;
>          }
>     -    entry->value = strdup(value);
>     -    if (!entry->value) {
>     -        free(entry->name);
>     -        free(entry);
>     -        return -ENOMEM;
>     +    if (!value) {
>     +        entry->value = NULL;
>     +    }
>     +     else {
>     +        entry->value = strdup(value);
>     +        if (!entry->value) {
>     +            free(entry->name);
>     +            free(entry);
>     +            return -ENOMEM;
>     +        }
>          }
>      
>          lastentry = NULL;
>     -- 
>     2.17.1
> 
> 
> -- 
> You received this message because you are subscribed to the Google
> Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to swupdate+unsubscribe@googlegroups.com
> <mailto:swupdate+unsubscribe@googlegroups.com>.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/swupdate/f8bd90e8-8d25-4e5b-bcd8-9bae98229825%40googlegroups.com
> <https://groups.google.com/d/msgid/swupdate/f8bd90e8-8d25-4e5b-bcd8-9bae98229825%40googlegroups.com?utm_medium=email&utm_source=footer>.
diff mbox series

Patch

diff --git a/src/fw_printenv.c b/src/fw_printenv.c
index 18887f9..8fd1f55 100644
--- a/src/fw_printenv.c
+++ b/src/fw_printenv.c
@@ -137,17 +137,21 @@  int main (int argc, char **argv) {
         if (!argc) {
             tmp = NULL;
             while ((tmp = libuboot_iterator(ctx, tmp)) != NULL) {
-                name = libuboot_getname(tmp);
                 value = libuboot_getvalue(tmp);
-                fprintf(stdout, "%s=%s\n", name, value);
+                if (value) {
+                    name = libuboot_getname(tmp);
+                    fprintf(stdout, "%s=%s\n", name, value);
+                }
             }
         } else {
             for (i = 0; i < argc; i++) {
                 value = libuboot_get_env(ctx, argv[i]);
-                if (noheader)
-                    fprintf(stdout, "%s\n", value ? value : "");
-                else
-                    fprintf(stdout, "%s=%s\n", argv[i], value ? value : 
"");
+                if (value) {
+                    if (noheader)
+                        fprintf(stdout, "%s\n", value ? value : "");
+                    else
+                        fprintf(stdout, "%s=%s\n", argv[i], value ? value 
: "");
+                }
             }
         }
     } else { /* setenv branch */
diff --git a/src/uboot_env.c b/src/uboot_env.c
index 09d210d..bf60635 100644
--- a/src/uboot_env.c
+++ b/src/uboot_env.c
@@ -735,6 +735,9 @@  int libuboot_env_store(struct uboot_ctx *ctx)
     uint8_t offsetdata;
     int ret;
     int copy;
+#if !defined(NDEBUG)
+    char * flagsbuf;
+#endif
 
     /*
      * Allocate the bigger of the case
@@ -752,21 +755,27 @@  int libuboot_env_store(struct uboot_ctx *ctx)
 
     buf = data;
     LIST_FOREACH(entry, &ctx->varlist, next) {
-        size = (ctx->size - offsetdata)  - (buf - data); 
-        if ((strlen(entry->name) + strlen(entry->value) + 2) > size) 
-            return -ENOMEM;
+        if (entry->value) {
+            /* only save variables which have a value (do exist yet) .. */
+            size = (ctx->size - offsetdata)  - (buf - data); 
+            if ((strlen(entry->name) + strlen(entry->value) + 2) > size) 
+                return -ENOMEM;
 
+            buf += snprintf(buf, size, "%s=%s", entry->name, 
entry->value); 
+            buf++;
+        }
+        /* .. but include non yet existing vars into .flags string */
         if (entry->type || entry->access)
             saveflags = true;
-
-        buf += snprintf(buf, size, "%s=%s", entry->name, entry->value); 
-        buf++;
     }
 
     /*
      * Now save the .flags
      */
     if (saveflags) {
+#if !defined(NDEBUG)
+        flagsbuf = buf;
+#endif
         bool first = true;
         size = (ctx->size - offsetdata)  - (buf - data);