| Message ID | 20200729193627.22906-1-liu.ming50@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [libubootenv] Dont store to device if no value changes | expand |
Hi Ming, On 29.07.20 21:36, liu.ming50@gmail.com wrote: > From: Ming Liu <liu.ming50@gmail.com> > > When fw_setenv is called, it could happen that the new value is same > with the old one, in which case, we should avoid storing data to > device. > Just note that this is different as in the past, because old tools always wrote to flash. But agree we can add the feature, please just extend the commit message with note about different behavior. Some projects used to run "fw_setenv" twice to be sure that both environemnt are identical. I do not think this was necessary, but again just write a comment that it does not work as before. > Signed-off-by: Ming Liu <liu.ming50@gmail.com> > --- > src/fw_printenv.c | 11 ++++++++--- > src/uboot_env.c | 12 ++++++++++-- > 2 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/src/fw_printenv.c b/src/fw_printenv.c > index 18887f9..cabeae5 100644 > --- a/src/fw_printenv.c > +++ b/src/fw_printenv.c > @@ -151,17 +151,22 @@ int main (int argc, char **argv) { > } > } > } else { /* setenv branch */ > + bool ok_change = false, ok_write = false; > if (scriptfile) > libuboot_load_file(ctx, scriptfile); > else { > for (i = 0; i < argc; i += 2) { > if (i + 1 == argc) > - libuboot_set_env(ctx, argv[i], NULL); This is to *remove* a variable. > + ok_write = (libuboot_set_env(ctx, argv[i], NULL) == 0); > else No. You are changing the library API. It is legitime to call libuboot_set_env() many times, even with the same value. The library handles this and does not create multiple variables. Please note that variables are just stored in memory. The caller of the library decide when to store. So returning -EAGAIN is just a trick to be informed if the value is the same (EAGAIN, why ?). Instead of this, I suggest to make changes only in fw_printenv.c, that is by implementing a read-modify-write approach. - First, call libuboot_get_env() - compare value, if differs call libuboot_set_env() - mark if you need to save as you are doing it with a bool > - libuboot_set_env(ctx, argv[i], argv[i+1]); > + ok_write = (libuboot_set_env(ctx, argv[i], argv[i+1]) == 0); > + if (ok_write) > + ok_change = true; > } > } > - ret = libuboot_env_store(ctx); > + > + if (ok_change) > + ret = libuboot_env_store(ctx); > if (ret) > fprintf(stderr, "Error storing the env\n"); > } > diff --git a/src/uboot_env.c b/src/uboot_env.c > index 25de4fb..c342931 100644 > --- a/src/uboot_env.c > +++ b/src/uboot_env.c > @@ -1242,8 +1242,16 @@ int libuboot_set_env(struct uboot_ctx *ctx, const char *varname, const char *val > if (!value) { > free_var_entry(envs, entry); > } else { > - free(entry->value); > - entry->value = strdup(value); > + if (strcmp(entry->value, value) == 0) { > +#if !defined(NDEBUG) > + fprintf(stdout, "No change to parameter, old %s, new %s\n", > + entry->value, value); > +#endif > + return -EAGAIN; This change is not necessary and breaks API. > + } else { > + free(entry->value); > + entry->value = strdup(value); > + } > } > return 0; > } else { > Best regards, Stefano Babic
Thanks for the review, will make a V2 as you suggested. //Ming Liu Stefano Babic <sbabic@denx.de> 於 2020年7月30日 週四 上午10:46寫道： > Hi Ming, > > On 29.07.20 21:36, liu.ming50@gmail.com wrote: > > From: Ming Liu <liu.ming50@gmail.com> > > > > When fw_setenv is called, it could happen that the new value is same > > with the old one, in which case, we should avoid storing data to > > device. > > > > Just note that this is different as in the past, because old tools > always wrote to flash. But agree we can add the feature, please just > extend the commit message with note about different behavior. > > Some projects used to run "fw_setenv" twice to be sure that both > environemnt are identical. I do not think this was necessary, but again > just write a comment that it does not work as before. > > > Signed-off-by: Ming Liu <liu.ming50@gmail.com> > > --- > > src/fw_printenv.c | 11 ++++++++--- > > src/uboot_env.c | 12 ++++++++++-- > > 2 files changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/src/fw_printenv.c b/src/fw_printenv.c > > index 18887f9..cabeae5 100644 > > --- a/src/fw_printenv.c > > +++ b/src/fw_printenv.c > > @@ -151,17 +151,22 @@ int main (int argc, char **argv) { > > } > > } > > } else { /* setenv branch */ > > + bool ok_change = false, ok_write = false; > > if (scriptfile) > > libuboot_load_file(ctx, scriptfile); > > else { > > for (i = 0; i < argc; i += 2) { > > if (i + 1 == argc) > > - libuboot_set_env(ctx, argv[i], > NULL); > > This is to *remove* a variable. > > > + ok_write = (libuboot_set_env(ctx, > argv[i], NULL) == 0); > > else > > No. You are changing the library API. It is legitime to call > libuboot_set_env() many times, even with the same value. The library > handles this and does not create multiple variables. Please note that > variables are just stored in memory. The caller of the library decide > when to store. So returning -EAGAIN is just a trick to be informed if > the value is the same (EAGAIN, why ?). Instead of this, I suggest to > make changes only in fw_printenv.c, that is by implementing a > read-modify-write approach. > > - First, call libuboot_get_env() > - compare value, if differs call libuboot_set_env() > - mark if you need to save as you are doing it with a bool > > > - libuboot_set_env(ctx, argv[i], > argv[i+1]); > > + ok_write = (libuboot_set_env(ctx, > argv[i], argv[i+1]) == 0); > > + if (ok_write) > > + ok_change = true; > > } > > } > > - ret = libuboot_env_store(ctx); > > + > > + if (ok_change) > > + ret = libuboot_env_store(ctx); > > if (ret) > > fprintf(stderr, "Error storing the env\n"); > > } > > diff --git a/src/uboot_env.c b/src/uboot_env.c > > index 25de4fb..c342931 100644 > > --- a/src/uboot_env.c > > +++ b/src/uboot_env.c > > @@ -1242,8 +1242,16 @@ int libuboot_set_env(struct uboot_ctx *ctx, const > char *varname, const char *val > > if (!value) { > > free_var_entry(envs, entry); > > } else { > > - free(entry->value); > > - entry->value = strdup(value); > > + if (strcmp(entry->value, value) == 0) { > > +#if !defined(NDEBUG) > > + fprintf(stdout, "No change to > parameter, old %s, new %s\n", > > + entry->value, value); > > +#endif > > + return -EAGAIN; > > This change is not necessary and breaks API. > > > + } else { > > + free(entry->value); > > + entry->value = strdup(value); > > + } > > } > > return 0; > > } else { > > > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ===================================================================== >
diff --git a/src/fw_printenv.c b/src/fw_printenv.c index 18887f9..cabeae5 100644 --- a/src/fw_printenv.c +++ b/src/fw_printenv.c @@ -151,17 +151,22 @@ int main (int argc, char **argv) { } } } else { /* setenv branch */ + bool ok_change = false, ok_write = false; if (scriptfile) libuboot_load_file(ctx, scriptfile); else { for (i = 0; i < argc; i += 2) { if (i + 1 == argc) - libuboot_set_env(ctx, argv[i], NULL); + ok_write = (libuboot_set_env(ctx, argv[i], NULL) == 0); else - libuboot_set_env(ctx, argv[i], argv[i+1]); + ok_write = (libuboot_set_env(ctx, argv[i], argv[i+1]) == 0); + if (ok_write) + ok_change = true; } } - ret = libuboot_env_store(ctx); + + if (ok_change) + ret = libuboot_env_store(ctx); if (ret) fprintf(stderr, "Error storing the env\n"); } diff --git a/src/uboot_env.c b/src/uboot_env.c index 25de4fb..c342931 100644 --- a/src/uboot_env.c +++ b/src/uboot_env.c @@ -1242,8 +1242,16 @@ int libuboot_set_env(struct uboot_ctx *ctx, const char *varname, const char *val if (!value) { free_var_entry(envs, entry); } else { - free(entry->value); - entry->value = strdup(value); + if (strcmp(entry->value, value) == 0) { +#if !defined(NDEBUG) + fprintf(stdout, "No change to parameter, old %s, new %s\n", + entry->value, value); +#endif + return -EAGAIN; + } else { + free(entry->value); + entry->value = strdup(value); + } } return 0; } else {