| Message ID | 58ceb541-8917-4316-a4c4-2a0f3650e65co@googlegroups.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [meta-swupdate,libubootenv] Parse .flags variable also when environment is read from default file (Bugfix) | expand |
Hi Uwe, On 05.06.20 16:06, Uwe Schuster wrote: > From f864bc0d8b96837e2a0ddc4e2ecd9193346a72f8 Mon Sep 17 00:00:00 2001 > From: Uwe Schuster <uwe.schuster@woodward.com> > Date: Tue, 2 Jun 2020 13:33:58 +0200 > Subject: [PATCH] Parse .flags variable also when environment is read from > default file (Bugfix). > > When reading variables from the default env file, as it happens after > mtd area was erased or with -f command line flag given, the .flags > variable IS NOT parsed as it is done when reading from normal env flash > area. > I am quite confused. There are dicrepancies, look at this: http://patchwork.ozlabs.org/project/swupdate/patch/58ceb541-8917-4316-a4c4-2a0f3650e65co@googlegroups.com/ A part of the patch is inside the comment (and drop). The function libuboot_parseflags() seems the same as before (just changed the name), but the patch indicates that it was completely rewritten. Some issues with editor / mailer ? > Signed-off-by: Uwe Schuster <uwe.schuster@woodward.com> > --- > src/uboot_env.c | 154 > ++++++++++++++++++++++++++++++++------------------------ > 1 file changed, 88 insertions(+), 66 deletions(-) > > diff --git a/src/uboot_env.c b/src/uboot_env.c > index f9ffeda..2f7f714 100644 > --- a/src/uboot_env.c > +++ b/src/uboot_env.c > @@ -847,6 +847,73 @@ int libuboot_env_store(struct uboot_ctx *ctx) > return ret; > } > > +static void libuboot_parseflags(struct uboot_ctx *ctx, char* flagsvar) > +{ > + char *pvar; > + char *pval; > + struct var_entry *entry; > + > +#if !defined(NDEBUG) > + fprintf(stdout, "Environment FLAGS %s\n", flagsvar); > +#endif > + pvar = flagsvar; > + > + while (*pvar && (pvar - flagsvar) < strlen(flagsvar)) { > + char *pnext; > + pval = strchr(pvar, ':'); > + if (!pval) > + break; > + > + *pval++ = '\0'; > + pnext = strchr(pval, ','); > + if (!pnext) > + pnext = flagsvar + strlen(flagsvar); > + else > + *pnext++ = '\0'; > + > + 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; > + } > + } > + } > + pvar = pnext; > + } > +} > + Function is the same, something went wrong with patch. Please change just the lines you want to patch without rewriting the same code, else it is difficult to understand what was really changed. > static int libuboot_load(struct uboot_ctx *ctx) > { > int ret, i; > @@ -963,7 +1030,7 @@ static int libuboot_load(struct uboot_ctx *ctx) > > *value++ = '\0'; > > - if (!strcmp(line, ".flags")) > + if (0 == strcmp(line, ".flags")) Ok, I am not a fan of Yoda's conditions, and there is no line in code with it. And there is no change in meaning with the replaced compacter code. > flagsvar = strdup(value); > else > libuboot_set_env(ctx, line, value); > @@ -973,72 +1040,11 @@ static int libuboot_load(struct uboot_ctx *ctx) > /* > * Parse .flags and set the attributes for a variable > */ > - char *pvar; > - char *pval; > if (flagsvar) { > -#if !defined(NDEBUG) > - fprintf(stdout, "Environment FLAGS %s\n", flagsvar); > -#endif > - pvar = flagsvar; > - > - while (*pvar && (pvar - flagsvar) < strlen(flagsvar)) { > - char *pnext; > - pval = strchr(pvar, ':'); > - if (!pval) > - break; > - > - *pval++ = '\0'; > - pnext = strchr(pval, ','); > - if (!pnext) > - pnext = flagsvar + strlen(flagsvar); > - else > - *pnext++ = '\0'; > - > - 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; > - } > - } > - } > - > - pvar = pnext; > - } > + libuboot_parseflags(ctx, flagsvar); > } > - free(flagsvar); > > + free(flagsvar); > free(buf[0]); > > return ctx->valid ? 0 : -ENODATA; > @@ -1064,6 +1070,8 @@ int libuboot_load_file(struct uboot_ctx *ctx, > const char *filename) > return -ENOMEM; > } > > + char *flagsvar = NULL; > + > while (fgets(buf, LINE_LENGTH, fp)) { > int len = strlen(buf); > > @@ -1082,13 +1090,27 @@ int libuboot_load_file(struct uboot_ctx *ctx, > const char *filename) > > name = buf; > > - if (!strlen(value)) > + if (0 != strcmp(name, ".flags")) { Ditto. > + /* normal variable */ > + if (strlen(value)) > value = NULL; > > libuboot_set_env(ctx, name, value); > } > - fclose(fp); > + else /* process flags variable after while loop */ > + flagsvar = strdup(value); > + } > + > + /* > + * Parse .flags to set the attributes for all variables > + */ > + if (flagsvar) { > + libuboot_parseflags(ctx, flagsvar); > + } > + > + free(flagsvar); > free(buf); > + fclose(fp); > > return 0; > } > > Best regards, Stefano
Hi Stefano, the patch works fine for me on top of master branch. Am Mittwoch, 10. Juni 2020 12:08:12 UTC+2 schrieb Stefano Babic: > > Hi Uwe, > > On 05.06.20 16:06, Uwe Schuster wrote: > > > I am quite confused. There are dicrepancies, look at this: > > > http://patchwork.ozlabs.org/project/swupdate/patch/58ceb541-8917-4316-a4c4-2a0f3650e65co@googlegroups.com/ > > A part of the patch is inside the comment (and drop). The function > libuboot_parseflags() seems the same as before (just changed the name), > but the patch indicates that it was completely rewritten. Some issues > with editor / mailer ? > There was no libuboot_parseflags() function before the patch, it is created by me with code which was inside the function libuboot_load(). This is done because the same code is needed inside libuboot_load_file() TO FIX the bug described in the comment. To not just duplicate the same identical parse code to two locations, I moved it out into the new libuboot_parseflags() function to be called now from libuboot_load() and libuboot_load_file(). > Function is the same, something went wrong with patch. Please change > just the lines you want to patch without rewriting the same code, else > it is difficult to understand what was really changed. > > See my remark above. > static int libuboot_load(struct uboot_ctx *ctx) > > { > > int ret, i; > > @@ -963,7 +1030,7 @@ static int libuboot_load(struct uboot_ctx *ctx) > > > > *value++ = '\0'; > > > > - if (!strcmp(line, ".flags")) > > + if (0 == strcmp(line, ".flags")) > > Ok, I am not a fan of Yoda's conditions, and there is no line in code > with it. And there is no change in meaning with the replaced compacter > code. > For me or us it doesn't really matter which style of code you prefer as long as function is identical, so just choose what you prefer (and stay with the previous variant. When creating the patch I have overseen this unnecessary change. Uwe
On 10.06.20 13:37, Uwe Schuster wrote: > Hi Stefano, > > the patch works fine for me on top of master branch. It does not work here: Applying patch #1304168 using "git am" Description: [meta-swupdate,libubootenv] Parse .flags variable also when environment is read from default file (Bugfix) Applying: Parse .flags variable also when environment is read from default file (Bugfix). error: patch fragment without header at line 7: @@ -1082,13 +1090,27 @@ int libuboot_load_file(struct uboot_ctx *ctx, const Patch failed at 0001 Parse .flags variable also when environment is read from default file (Bugfix). hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". 'git am' failed with exit status 128 And if I try manually: patching file src/uboot_env.c Hunk #1 succeeded at 847 with fuzz 1. Hunk #2 FAILED at 1030. Hunk #3 FAILED at 1040. patch: **** malformed patch at line 317: char *filename) > > > Am Mittwoch, 10. Juni 2020 12:08:12 UTC+2 schrieb Stefano Babic: > > Hi Uwe, > > On 05.06.20 16:06, Uwe Schuster wrote: > > > I am quite confused. There are dicrepancies, look at this: > > http://patchwork.ozlabs.org/project/swupdate/patch/58ceb541-8917-4316-a4c4-2a0f3650e65co@googlegroups.com/ > <http://patchwork.ozlabs.org/project/swupdate/patch/58ceb541-8917-4316-a4c4-2a0f3650e65co@googlegroups.com/> > > > A part of the patch is inside the comment (and drop). The function > libuboot_parseflags() seems the same as before (just changed the name), > but the patch indicates that it was completely rewritten. Some issues > with editor / mailer ? > > > There was no libuboot_parseflags() function before the patch, Right, but if I change the name of the function without modifying it, the resulting patch is: diff --git a/src/uboot_env.c b/src/uboot_env.c index f9ffeda..4b3c428 100644 --- a/src/uboot_env.c +++ b/src/uboot_env.c @@ -847,7 +847,7 @@ int libuboot_env_store(struct uboot_ctx *ctx) return ret; } -static int libuboot_load(struct uboot_ctx *ctx) +static void libuboot_parseflags(struct uboot_ctx *ctx, char* flagsvar) { int ret, i; int copies = 1; In your case something went wrong. > it is > created by me with code which was inside the function libuboot_load(). See above. > This is done because the same code is needed inside libuboot_load_file() > TO FIX the bug described in the comment. To not just duplicate the same > identical parse code to two locations, I moved it out into the new > libuboot_parseflags() function to be called now from libuboot_load() and > libuboot_load_file(). My comment points to the (formal) format of the patch. The patch should reflect just the changed lines. > > > Function is the same, something went wrong with patch. Please change > just the lines you want to patch without rewriting the same code, else > it is difficult to understand what was really changed. > > See my remark above. > > > static int libuboot_load(struct uboot_ctx *ctx) > > { > > int ret, i; > > @@ -963,7 +1030,7 @@ static int libuboot_load(struct uboot_ctx *ctx) > > > > *value++ = '\0'; > > > > - if (!strcmp(line, ".flags")) > > + if (0 == strcmp(line, ".flags")) > > Ok, I am not a fan of Yoda's conditions, and there is no line in code > with it. And there is no change in meaning with the replaced > compacter code. > > > For me or us it doesn't really matter which style of code you prefer It does matter when I upstream - the project must be as much consistent as possible, with just few exceptions. Else it will mix quick a lot of different coding styles and becomes a mess. > as > long as function is identical, so just choose what you prefer (and stay > with the previous variant. > When creating the patch I have overseen this unnecessary change. > Best regards, Stefano > Uwe > > -- > 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/040dd943-5837-4468-9f18-54c1b23b426eo%40googlegroups.com > <https://groups.google.com/d/msgid/swupdate/040dd943-5837-4468-9f18-54c1b23b426eo%40googlegroups.com?utm_medium=email&utm_source=footer>.
diff --git a/src/uboot_env.c b/src/uboot_env.c index f9ffeda..2f7f714 100644 --- a/src/uboot_env.c +++ b/src/uboot_env.c @@ -847,6 +847,73 @@ int libuboot_env_store(struct uboot_ctx *ctx) return ret; } +static void libuboot_parseflags(struct uboot_ctx *ctx, char* flagsvar) +{ + char *pvar; + char *pval; + struct var_entry *entry; + +#if !defined(NDEBUG) + fprintf(stdout, "Environment FLAGS %s\n", flagsvar); +#endif + pvar = flagsvar; + + while (*pvar && (pvar - flagsvar) < strlen(flagsvar)) { + char *pnext; + pval = strchr(pvar, ':'); + if (!pval) + break; + + *pval++ = '\0'; + pnext = strchr(pval, ','); + if (!pnext) + pnext = flagsvar + strlen(flagsvar); + else + *pnext++ = '\0'; + + 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; + } + } + } + pvar = pnext; + } +} + static int libuboot_load(struct uboot_ctx *ctx) { int ret, i; @@ -963,7 +1030,7 @@ static int libuboot_load(struct uboot_ctx *ctx) *value++ = '\0'; - if (!strcmp(line, ".flags")) + if (0 == strcmp(line, ".flags")) flagsvar = strdup(value); else libuboot_set_env(ctx, line, value); @@ -973,72 +1040,11 @@ static int libuboot_load(struct uboot_ctx *ctx) /* * Parse .flags and set the attributes for a variable */ - char *pvar; - char *pval; if (flagsvar) { -#if !defined(NDEBUG) - fprintf(stdout, "Environment FLAGS %s\n", flagsvar); -#endif - pvar = flagsvar; - - while (*pvar && (pvar - flagsvar) < strlen(flagsvar)) { - char *pnext; - pval = strchr(pvar, ':'); - if (!pval) - break; - - *pval++ = '\0'; - pnext = strchr(pval, ','); - if (!pnext) - pnext = flagsvar + strlen(flagsvar); - else - *pnext++ = '\0'; - - 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; - } - } - } - - pvar = pnext; - } + libuboot_parseflags(ctx, flagsvar); } - free(flagsvar); + free(flagsvar); free(buf[0]); return ctx->valid ? 0 : -ENODATA; @@ -1064,6 +1070,8 @@ int libuboot_load_file(struct uboot_ctx *ctx, const char *filename) return -ENOMEM; } + char *flagsvar = NULL; + while (fgets(buf, LINE_LENGTH, fp)) { int len = strlen(buf);