diff mbox series

[meta-swupdate,libubootenv] Add correct handling of 'o' (write once) and any other access or type flags if the variable which has the flag is part of the .flags value string but does not exist as a valued variable yet (variable not created

Message ID 740689eb-7833-4114-86a7-cb850fc371deo@googlegroups.com
State Changes Requested
Headers show
Series [meta-swupdate,libubootenv] Add correct handling of 'o' (write once) and any other access or type flags if the variable which has the flag is part of the .flags value string but does not exist as a valued variable yet (variable not created | expand

Commit Message

Uwe Schuster June 5, 2020, 2:08 p.m. UTC 
From 7766312aa1fbfa8d807ab3ded05cbd1db71440c9 Mon Sep 17 00:00:00 2001
From: Uwe Schuster <uwe.schuster@woodward.com>
Date: Tue, 2 Jun 2020 15:32:10 +0200
Subject: [PATCH] Add correct handling of 'o' (write once) and any other 
access
 or type flags if the variable which has the flag is part of the .flags 
value
 string but does not exist as a valued variable yet (variable not created 
yet
 and value assigned) (bugfix).

With the fix you can specify variables in the .flags variable which don't 
need to exist as a variable with a value assigned, but can be created later 
and after creation will obey their flags given earlier. Especially write 
once variables can now be created once and not changed after.
The only issue which still exists even with this fix: when using 
fw_printenv it will not include .flags variable in the printed list 
(despite its there). Because of the chosen flag handling implementation any 
solution for that would require much not easy changes in the printenv code.

Signed-off-by: Uwe Schuster <uwe.schuster@woodward.com>
---
 src/fw_printenv.c |  16 +++--
 src/uboot_env.c   | 173 
+++++++++++++++++++++++++++++++++++-------------------
 2 files changed, 124 insertions(+), 65 deletions(-)

 static int libuboot_load(struct uboot_ctx *ctx)
@@ -1095,7 +1119,7 @@ int libuboot_load_file(struct uboot_ctx *ctx, const 
char *filename)
         if (strlen(value))
             value = NULL;
 
-        libuboot_set_env(ctx, name, value);
+                libuboot_set_env(ctx, name, value);
     }
     else /* process flags variable after while loop */
         flagsvar = strdup(value);
@@ -1205,12 +1229,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;
@@ -1243,6 +1274,10 @@ static bool libuboot_validate_flags(struct var_entry 
*entry, const char *value)
     case TYPE_ATTR_MAC:
         break;
     }
+#if !defined(NDEBUG)
+    fprintf(stdout, "Type flag validation %s\n", ok_type ? "success." : 
"failed.");
+#endif
+
     return ok_type;
 }
 
@@ -1250,13 +1285,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;
@@ -1265,9 +1317,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;
@@ -1276,12 +1325,18 @@ 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;
     LIST_FOREACH(elm, envs, next) {
         if (strcmp(elm->name, varname) > 0) {

Comments

Stefano Babic June 10, 2020, 10:16 a.m. UTC | #1 
Hi Uwe,

On 05.06.20 16:08, Uwe Schuster wrote:
> From 7766312aa1fbfa8d807ab3ded05cbd1db71440c9 Mon Sep 17 00:00:00 2001
> From: Uwe Schuster <uwe.schuster@woodward.com>
> Date: Tue, 2 Jun 2020 15:32:10 +0200
> Subject: [PATCH] Add correct handling of 'o' (write once) and any other
> access
>  or type flags if the variable which has the flag is part of the .flags
> value
>  string but does not exist as a valued variable yet (variable not
> created yet
>  and value assigned) (bugfix).
> 
> With the fix you can specify variables in the .flags variable which
> don't need to exist as a variable with a value assigned, but can be
> created later and after creation will obey their flags given earlier.
> Especially write once variables can now be created once and not changed
> after.
> The only issue which still exists even with this fix: when using
> fw_printenv it will not include .flags variable in the printed list
> (despite its there). Because of the chosen flag handling implementation
> any solution for that would require much not easy changes in the
> printenv code.
> 
> Signed-off-by: Uwe Schuster <uwe.schuster@woodward.com>
> ---
>  src/fw_printenv.c |  16 +++--
>  src/uboot_env.c   | 173
> +++++++++++++++++++++++++++++++++++-------------------
>  2 files changed, 124 insertions(+), 65 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);
> +                }

This has nothing to do with the flags, it should be addressed by a
separate patch. But which is the issue you want to fix here ?

If a variable is not set, it is removed by the list and it does not
appear when printed. IMHO this above does not make sense.

>              }
>          } 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 : "");

Where is the difference introduced by patch ?

> +                }
>              }
>          }
>      } else { /* setenv branch */
> diff --git a/src/uboot_env.c b/src/uboot_env.c
> index 2f7f714..1d9b0f9 100644
> --- a/src/uboot_env.c
> +++ b/src/uboot_env.c
> @@ -765,6 +765,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
> @@ -782,21 +785,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=");
> @@ -809,11 +818,16 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>                          entry->name,
>                          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) {
> @@ -847,18 +861,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)
> @@ -871,47 +886,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;
>              }

Same problem reported before - same code, some issues with editor, it
looks new code, it is not.

>          }
> +
>          pvar = pnext;
>      }
> +    free(flagsvar);
>  }
>  
>  static int libuboot_load(struct uboot_ctx *ctx)
> @@ -1095,7 +1119,7 @@ int libuboot_load_file(struct uboot_ctx *ctx,
> const char *filename)
>          if (strlen(value))
>              value = NULL;
>  
> -        libuboot_set_env(ctx, name, value);
> +                libuboot_set_env(ctx, name, value);
>      }
>      else /* process flags variable after while loop */
>          flagsvar = strdup(value);
> @@ -1205,12 +1229,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;
> @@ -1243,6 +1274,10 @@ static bool libuboot_validate_flags(struct
> var_entry *entry, const char *value)
>      case TYPE_ATTR_MAC:
>          break;
>      }
> +#if !defined(NDEBUG)
> +    fprintf(stdout, "Type flag validation %s\n", ok_type ? "success." :
> "failed.");
> +#endif
> +
>      return ok_type;
>  }
>  
> @@ -1250,13 +1285,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;
> @@ -1265,9 +1317,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;
> @@ -1276,12 +1325,18 @@ 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;
>      LIST_FOREACH(elm, envs, next) {
>          if (strcmp(elm->name, varname) > 0) {
> 

Best regards,
Stefano Babic
Uwe Schuster June 10, 2020, 12:33 p.m. UTC | #2 
Hi Stefano,

Am Mittwoch, 10. Juni 2020 12:16:41 UTC+2 schrieb Stefano Babic:
>
> Hi Uwe, 
>
> On 05.06.20 16:08, Uwe Schuster wrote: 
> > From 7766312aa1fbfa8d807ab3ded05cbd1db71440c9 Mon Sep 17 00:00:00 2001 
>
> > Subject: [PATCH] Add correct handling of 'o' (write once) and any other 
> > access 
> >  or type flags if the variable which has the flag is part of the .flags 
> > value 
> >  string but does not exist as a valued variable yet (variable not 
> > created yet 
> >  and value assigned) (bugfix). 
> > 
> > With the fix you can specify variables in the .flags variable which 
> > don't need to exist as a variable with a value assigned, but can be 
> > created later and after creation will obey their flags given earlier. 
> > Especially write once variables can now be created once and not changed 
> > after. 
> > The only issue which still exists even with this fix: when using 
> > fw_printenv it will not include .flags variable in the printed list 
> > (despite its there). Because of the chosen flag handling implementation 
> > any solution for that would require much not easy changes in the 
> > printenv code. 
> > 
>
> > 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); 
> > +                } 
>
> This has nothing to do with the flags, it should be addressed by a 
> separate patch. But which is the issue you want to fix here ? 
>
> If a variable is not set, it is removed by the list and it does not 
> appear when printed. IMHO this above does not make sense. 
>

It has to do with the patch and how I decided to implement the fix for it. 
The existing code does not anywhere deal with variables which do exist by 
name but without any value. There was no need yet. 
To implement a fix for the mentioned behavior I saw two different 
approaches for implementation. To change as little as possible in the 
existing implementation I decided against first approach by adding some 
more meta data to the variable struct to mark a variable as existing yet or 
not. Instead I used second approach - the value member of a variable is 
used as the indicator for such kind of variables. If value == NULL then the 
variable exists (as part of .flags) but has no value assigned yet. But this 
choosen implementation requires some small changes on places like the 
print_env function - it shall NOT print out variables which don't have a 
value assigned yet - they only exist in the internal database so that any 
storage of env will rebuild and store the .flags variable as it was before 
when read in. 


> >              } 
> >          } 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 : ""); 
>
> Where is the difference introduced by patch ? 
>

Difference is the  if (value) clause - see my comment above.


> > +                } 
> >              } 
> >          } 
> >      } else { /* setenv branch */ 
> > diff --git a/src/uboot_env.c b/src/uboot_env.c 
> > index 2f7f714..1d9b0f9 100644 
> > --- a/src/uboot_env.c 
> > +++ b/src/uboot_env.c 
> > @@ -765,6 +765,9 @@ int libuboot_env_store(struct uboot_ctx *ctx) 
> >      uint8_t offsetdata; 
> >      int ret; 
> >      int copy; 
> > +#if !defined(NDEBUG) 
> > +    char * flagsbuf; 
> > +#endif 
>
> This was added only for debug reasons the see whats going on with the 
.flags variable behind the scenes when read and written to env.

>
> >   
> >      /* 
> >       * Allocate the bigger of the case 
> > @@ -782,21 +785,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="); 
> > @@ -809,11 +818,16 @@ int libuboot_env_store(struct uboot_ctx *ctx) 
> >                          entry->name, 
> >                          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) { 
> > @@ -847,18 +861,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) 
> > @@ -871,47 +886,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; 
> >              } 
>
> Same problem reported before - same code, some issues with editor, it 
> looks new code, it is not. 
>

The difference here is the indentation level of the code - one less than 
before. However it can be made the same as before by not removing "if 
(entry) {" line. However I did remove it because with added if(!entry) 
clause, entry is always !=NULL now.


Best Regards
Uwe
Stefano Babic June 10, 2020, 1:35 p.m. UTC | #3 
Hi Uwe,

On 10.06.20 14:33, Uwe Schuster wrote:
> Hi Stefano,
> 
> Am Mittwoch, 10. Juni 2020 12:16:41 UTC+2 schrieb Stefano Babic:
> 
>     Hi Uwe,
> 
>     On 05.06.20 16:08, Uwe Schuster wrote:
>     > From 7766312aa1fbfa8d807ab3ded05cbd1db71440c9 Mon Sep 17 00:00:00
>     2001
> 
>     > Subject: [PATCH] Add correct handling of 'o' (write once) and any
>     other
>     > access
>     >  or type flags if the variable which has the flag is part of the
>     .flags
>     > value
>     >  string but does not exist as a valued variable yet (variable not
>     > created yet
>     >  and value assigned) (bugfix).
>     >
>     > With the fix you can specify variables in the .flags variable which
>     > don't need to exist as a variable with a value assigned, but can be
>     > created later and after creation will obey their flags given earlier.
>     > Especially write once variables can now be created once and not
>     changed
>     > after.
>     > The only issue which still exists even with this fix: when using
>     > fw_printenv it will not include .flags variable in the printed list
>     > (despite its there). Because of the chosen flag handling
>     implementation
>     > any solution for that would require much not easy changes in the
>     > printenv code.
>     >
> 
>     > 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);
>     > +                }
> 
>     This has nothing to do with the flags, it should be addressed by a
>     separate patch. But which is the issue you want to fix here ?
> 
>     If a variable is not set, it is removed by the list and it does not
>     appear when printed. IMHO this above does not make sense.
> 
> 
> It has to do with the patch and how I decided to implement the fix for
> it. 

Sorry, I have not understood this from commit message (write once flag).

The existing code does not anywhere deal with variables which do
> exist by name but without any value. There was no need yet.

mmmh...ok, I have to think about. This is also the way used by U-Boot to
drop variables. In U-Boot, I use:

	setenv dummy 1	==> set variable
	setenv dummy	==> drop variable

And the implementation in libubootenv corresponds to the behavior in
U-Boot. Which is the use case to have a variable that points to a Null
(not empty) value ? And cannot be this use case handled in U-Boot
scripts (bootloader) or evaluating the result of fw_printenv in User
Space (linux) ?

In u-boot, scripts can use the "exists" clause, something like "env
exists dummy && ". In user space, it is enough to check the return value
of fw_printenv or evaluating the output.

> To implement a fix for the mentioned behavior I saw two different
> approaches for implementation. To change as little as possible in the
> existing implementation I decided against first approach by adding some
> more meta data to the variable struct to mark a variable as existing yet
> or not.

Let me know your use case, that is a variable without a value - I have
not yet understood it.

How do we distinguish between dropping variable and setting variable to
Null (if second case makes sense, of course) ?

> Instead I used second approach - the value member of a variable
> is used as the indicator for such kind of variables. If value == NULL
> then the variable exists (as part of .flags) but has no value assigned
> yet.

mmmhh...ok, I understand now the point: .flags contains the variable as
write-once. I needed my time.

Then I do not like the implementation, it looks like a trick. I would
prefer that library adds an own structure for this "not-yet" elaborated
flags, and this is parsed and evaluated when a set is done. If a
write-once for the variable is found, then the first set is accepted and
the entry is removed by the not yet-elaborated list.

> But this choosen implementation requires some small changes on
> places like the print_env function - it shall NOT print out variables
> which don't have a value assigned yet - they only exist in the internal
> database so that any storage of env will rebuild and store the .flags
> variable as it was before when read in.
> 
> 
>     >              }
>     >          } 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 : "");
> 
>     Where is the difference introduced by patch ?
> 
> 
> Difference is the  if (value) clause - see my comment above.

Ok, now it is clear to me, but it sounds like a hack.

> 
> 
>     > +                }
>     >              }
>     >          }
>     >      } else { /* setenv branch */
>     > diff --git a/src/uboot_env.c b/src/uboot_env.c
>     > index 2f7f714..1d9b0f9 100644
>     > --- a/src/uboot_env.c
>     > +++ b/src/uboot_env.c
>     > @@ -765,6 +765,9 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>     >      uint8_t offsetdata;
>     >      int ret;
>     >      int copy;
>     > +#if !defined(NDEBUG)
>     > +    char * flagsbuf;
>     > +#endif
> 
> This was added only for debug reasons the see whats going on with the
> .flags variable behind the scenes when read and written to env.

Ok

> 
> 
>     >  
>     >      /*
>     >       * Allocate the bigger of the case
>     > @@ -782,21 +785,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=");
>     > @@ -809,11 +818,16 @@ int libuboot_env_store(struct uboot_ctx *ctx)
>     >                          entry->name,
>     >                          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) {
>     > @@ -847,18 +861,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)
>     > @@ -871,47 +886,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;
>     >              }
> 
>     Same problem reported before - same code, some issues with editor, it
>     looks new code, it is not.
> 
> 
> The difference here is the indentation level of the code - one less than
> before. However it can be made the same as before by not removing "if
> (entry) {" line. However I did remove it because with added if(!entry)
> clause, entry is always !=NULL now.
> 
Best regards,
Stefano
Uwe Schuster June 18, 2020, 4:02 p.m. UTC | #4 
Hi Stefano,

sorry for the long delay, been busy other while.

Am Mittwoch, 10. Juni 2020 15:35:19 UTC+2 schrieb Stefano Babic:
>
> Hi Uwe, 
>
> On 10.06.20 14:33, Uwe Schuster wrote: 
> > Hi Stefano, 
> > 
> > Am Mittwoch, 10. Juni 2020 12:16:41 UTC+2 schrieb Stefano Babic: 
> > 
> >     Hi Uwe, 
> > 
> >     On 05.06.20 16:08, Uwe Schuster wrote: 
> >     > From 7766312aa1fbfa8d807ab3ded05cbd1db71440c9 Mon Sep 17 00:00:00 
> >     2001 
> > 
> >     > Subject: [PATCH] Add correct handling of 'o' (write once) and any 
> >     other 
> >     > access 
> >     >  or type flags if the variable which has the flag is part of the 
> >     .flags 
> >     > value 
> >     >  string but does not exist as a valued variable yet (variable not 
> >     > created yet 
> >     >  and value assigned) (bugfix). 
> >     > 
> >     > With the fix you can specify variables in the .flags variable 
> which 
> >     > don't need to exist as a variable with a value assigned, but can 
> be 
> >     > created later and after creation will obey their flags given 
> earlier. 
> >     > Especially write once variables can now be created once and not 
> >     changed 
> >     > after. 
> >     > The only issue which still exists even with this fix: when using 
> >     > fw_printenv it will not include .flags variable in the printed 
> list 
> >     > (despite its there). Because of the chosen flag handling 
> >     implementation 
> >     > any solution for that would require much not easy changes in the 
> >     > printenv code. 
> >     > 
> > 
> >     > 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); 
> >     > +                } 
> > 
> >     This has nothing to do with the flags, it should be addressed by a 
> >     separate patch. But which is the issue you want to fix here ? 
> > 
> >     If a variable is not set, it is removed by the list and it does not 
> >     appear when printed. IMHO this above does not make sense. 
> > 
> > 
> > It has to do with the patch and how I decided to implement the fix for 
> > it. 
>
> Sorry, I have not understood this from commit message (write once flag). 
>
> The existing code does not anywhere deal with variables which do 
> > exist by name but without any value. There was no need yet. 
>
> mmmh...ok, I have to think about. This is also the way used by U-Boot to 
> drop variables. In U-Boot, I use: 
>
>         setenv dummy 1        ==> set variable 
>         setenv dummy        ==> drop variable 
>
> And the implementation in libubootenv corresponds to the behavior in 
> U-Boot. Which is the use case to have a variable that points to a Null 
> (not empty) value ? And cannot be this use case handled in U-Boot 
> scripts (bootloader) or evaluating the result of fw_printenv in User 
> Space (linux) ? 
>
> In u-boot, scripts can use the "exists" clause, something like "env 
> exists dummy && ". In user space, it is enough to check the return value 
> of fw_printenv or evaluating the output. 
>
> > To implement a fix for the mentioned behavior I saw two different 
> > approaches for implementation. To change as little as possible in the 
> > existing implementation I decided against first approach by adding some 
> > more meta data to the variable struct to mark a variable as existing yet 
> > or not. 
>
> Let me know your use case, that is a variable without a value - I have 
> not yet understood it. 
>
> How do we distinguish between dropping variable and setting variable to 
> Null (if second case makes sense, of course) ? 
>

Our use case is as follows: in the production process the device will get 
its firmware deployed then it will be powered up into Linux and some remote 
running scripts will use swupdate/fw_setenv facilities to set some env 
variables in uboot to its final values - one of them is "ethaddr". To 
achieve this, the initial firmware is deployed with a default environment 
file, containing a .flags variable which itself contains "ethaddr" variable 
marked with "mo" flags but without a value. Then the scripts set the value 
of this variable to a specific MAC address. With existing libubootenv this 
fails because first the default env file is not parsed correctly for .flags 
variable (which is addressed by the other patch posted) and second (if 
first is fixed) the ethaddr is not stored in internal database because no 
value is assigned yet when read. This would be fixed by this patch.

>
> > Instead I used second approach - the value member of a variable 
> > is used as the indicator for such kind of variables. If value == NULL 
> > then the variable exists (as part of .flags) but has no value assigned 
> > yet. 
>
> mmmhh...ok, I understand now the point: .flags contains the variable as 
> write-once. I needed my time. 
>
> Then I do not like the implementation, it looks like a trick. I would 
> prefer that library adds an own structure for this "not-yet" elaborated 
> flags, and this is parsed and evaluated when a set is done. If a 
> write-once for the variable is found, then the first set is accepted and 
> the entry is removed by the not yet-elaborated list. 
>

I'd not say its a real hack, since having a variable without a value could 
be imho really thought and implemented by having the value set to NULL. Of 
course there are other (nicer) implementations as you are mentioning one. I 
have considered those but decided against them, because they would require 
much more effort to be implemented and introduce more crucial changes in 
existing code. Which itself leads to the problem of testing. Since I could 
not spot any unit tests for this library I was concerned, that something 
else functionality gets broken because of any side effects etc.

>
> > But this choosen implementation requires some small changes on 
> > places like the print_env function - it shall NOT print out variables 
> > which don't have a value assigned yet - they only exist in the internal 
> > database so that any storage of env will rebuild and store the .flags 
> > variable as it was before when read in. 
> > 
> >     Where is the difference introduced by patch ? 
> > 
> > 
> > Difference is the  if (value) clause - see my comment above. 
>
> Ok, now it is clear to me, but it sounds like a hack. 
>
>
 I will not be able to rewrite the patch to implement your suggested 
approach because I'm already working for a different company. The old 
company just wanted to provide that fix for obviously wrong behavior and 
have it upstream, to make it easier for us to update to newer versions of 
swupdate in future. 

But it would be also appreciated and acceptable if you or someone else is 
going to fix the implementation as you suggest it. Important for us is only 
the correct behavior.

Regards
Uwe
 

>   
>
Stefano Babic June 18, 2020, 4:29 p.m. UTC | #5 
Hallo Uwe,

On 18.06.20 18:02, Uwe Schuster wrote:
> Hi Stefano,
> 
> sorry for the long delay, been busy other while.
> 
> Am Mittwoch, 10. Juni 2020 15:35:19 UTC+2 schrieb Stefano Babic:

[snip]

> 
> Our use case is as follows: in the production process the device will
> get its firmware deployed then it will be powered up into Linux and some
> remote running scripts will use swupdate/fw_setenv facilities to set
> some env variables in uboot to its final values - one of them is
> "ethaddr". To achieve this, the initial firmware is deployed with a
> default environment file, containing a .flags variable which itself
> contains "ethaddr" variable marked with "mo" flags but without a value.
> Then the scripts set the value of this variable to a specific MAC
> address. With existing libubootenv this fails because first the default
> env file is not parsed correctly for .flags variable (which is addressed
> by the other patch posted) and second (if first is fixed) the ethaddr is
> not stored in internal database because no value is assigned yet when
> read. This would be fixed by this patch.

Thanks for clarification - your use case is now clear to me.

> 
> 
>     > Instead I used second approach - the value member of a variable
>     > is used as the indicator for such kind of variables. If value == NULL
>     > then the variable exists (as part of .flags) but has no value
>     assigned
>     > yet.
> 
>     mmmhh...ok, I understand now the point: .flags contains the variable as
>     write-once. I needed my time.
> 
>     Then I do not like the implementation, it looks like a trick. I would
>     prefer that library adds an own structure for this "not-yet" elaborated
>     flags, and this is parsed and evaluated when a set is done. If a
>     write-once for the variable is found, then the first set is accepted
>     and
>     the entry is removed by the not yet-elaborated list.
> 
> 
> I'd not say its a real hack, since having a variable without a value
> could be imho really thought and implemented by having the value set to
> NULL.

This is against the logic supplied since from the beginning in U-Boot. A
variable cannot be set to NULL, and when a variable is set to nothing,
the variable is dropped (setenv behavior). I would like to maintain the
same logic here.

The other advantage about this implementation is that it is not
necessary to check values for a variable. If a variable is not in the
list, it does not exist.


> Of course there are other (nicer) implementations as you are
> mentioning one. I have considered those but decided against them,
> because they would require much more effort to be implemented and
> introduce more crucial changes in existing code.

I agree this requires more effort, but I am quite sure this lets better
and more maintainable code.

> Which itself leads to
> the problem of testing. Since I could not spot any unit tests for this
> library I was concerned, that something else functionality gets broken
> because of any side effects etc

Right, test must be considered, too.

.
> 
> 
>     > But this choosen implementation requires some small changes on
>     > places like the print_env function - it shall NOT print out variables
>     > which don't have a value assigned yet - they only exist in the
>     internal
>     > database so that any storage of env will rebuild and store the .flags
>     > variable as it was before when read in.
>     >
>     >     Where is the difference introduced by patch ?
>     >
>     >
>     > Difference is the  if (value) clause - see my comment above.
> 
>     Ok, now it is clear to me, but it sounds like a hack.
> 
> 
>  I will not be able to rewrite the patch to implement your suggested
> approach because I'm already working for a different company.

;-)

> The old
> company just wanted to provide that fix for obviously wrong behavior

Behavior with flags is wrong, fully agree at this point.

> and
> have it upstream, to make it easier for us to update to newer versions
> of swupdate in future.

..but to make it upstreamable, it should be also done in a way that can
be easier maintainable.

> 
> But it would be also appreciated and acceptable if you or someone else
> is going to fix the implementation as you suggest it. Important for us
> is only the correct behavior.

Fully agree about wrong behavior. In my case, I rework / fix when I have
a running project asking for this, and it is not yet the case (flags is
a feature not used very often). I would note this, or you can tell your
old company to contact me for this work.

Best regards,
Stefano
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 2f7f714..1d9b0f9 100644
--- a/src/uboot_env.c
+++ b/src/uboot_env.c
@@ -765,6 +765,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
@@ -782,21 +785,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=");
@@ -809,11 +818,16 @@  int libuboot_env_store(struct uboot_ctx *ctx)
                         entry->name,
                         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) {
@@ -847,18 +861,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)
@@ -871,47 +886,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);
 }