| Message ID | 20230408203952.2522758-1-offougajoris@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [1/1] Add support for new configuration file for libubootenv in YAML | expand |
Hi Joris, On 08.04.23 22:39, Joris Offouga wrote: > see https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c > > Signed-off-by: Joris Offouga <offougajoris@gmail.com> > --- > bootloader/uboot.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/bootloader/uboot.c b/bootloader/uboot.c > index a2f0794..e3a307f 100644 > --- a/bootloader/uboot.c > +++ b/bootloader/uboot.c > @@ -32,6 +32,7 @@ static struct { > int (*initialize)(struct uboot_ctx **out, struct uboot_env_device *envdevs); > char* (*get_env)(struct uboot_ctx *ctx, const char *varname); > int (*read_config)(struct uboot_ctx *ctx, const char *config); > + int (*read_multi_config)(struct uboot_ctx **ctxlist, const char *config); > int (*load_file)(struct uboot_ctx *ctx, const char *filename); > int (*set_env)(struct uboot_ctx *ctx, const char *varname, const char *value); > int (*env_store)(struct uboot_ctx *ctx); > @@ -43,9 +44,12 @@ static int bootloader_initialize(struct uboot_ctx **ctx) > ERROR("Error: environment not initialized"); > return -ENODEV; > } > - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { > - ERROR("Configuration file %s wrong or corrupted", CONFIG_UBOOT_FWENV); > - return -EINVAL; > + if (libuboot.read_multi_config(ctx, CONFIG_UBOOT_FWENV)) { > + WARN("Configuration file %s not in new format, fallback to legacy format", CONFIG_UBOOT_FWENV); > + if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { > + ERROR("Configuration file %s wrong or corrupted", CONFIG_UBOOT_FWENV); > + return -EINVAL; > + } > } > if (libuboot.open(*ctx) < 0) { > WARN("Cannot read environment, using default"); > @@ -130,6 +134,7 @@ static bootloader* probe(void) > libuboot.initialize = libuboot_initialize; > libuboot.get_env = libuboot_get_env; > libuboot.read_config = libuboot_read_config; > + libuboot.read_multi_config = libuboot_read_multi_config; > libuboot.load_file = libuboot_load_file; > libuboot.set_env = libuboot_set_env; > libuboot.env_store = libuboot_env_store; > @@ -146,6 +151,7 @@ static bootloader* probe(void) > load_symbol(handle, &libuboot.initialize, "libuboot_initialize"); > load_symbol(handle, &libuboot.get_env, "libuboot_get_env"); > load_symbol(handle, &libuboot.read_config, "libuboot_read_config"); > + load_symbol(handle, &libuboot.read_multi_config, "libuboot_read_multiple_config"); This does not work. It just work if libubootenv is pushed to the last version incluiding support for YAML. If libubootenv does not support it, load_symbol (macro) will close the handle and the library is closed. This is unwanted. Because library is dynamically loaded, it is supposed that SWUpdate can handle both, else an upgrade to SWUpdate will force an update to libubootenv, even if the configuration format is still legacy. > load_symbol(handle, &libuboot.load_file, "libuboot_load_file"); > load_symbol(handle, &libuboot.set_env, "libuboot_set_env"); > load_symbol(handle, &libuboot.env_store, "libuboot_env_store"); Best regards, Stefano
Hi Stefano, Le dim. 9 avr. 2023 à 12:25, Stefano Babic <sbabic@denx.de> a écrit : > Hi Joris, > > On 08.04.23 22:39, Joris Offouga wrote: > > see > https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c > > > > Signed-off-by: Joris Offouga <offougajoris@gmail.com> > > --- > > bootloader/uboot.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/bootloader/uboot.c b/bootloader/uboot.c > > index a2f0794..e3a307f 100644 > > --- a/bootloader/uboot.c > > +++ b/bootloader/uboot.c > > @@ -32,6 +32,7 @@ static struct { > > int (*initialize)(struct uboot_ctx **out, struct > uboot_env_device *envdevs); > > char* (*get_env)(struct uboot_ctx *ctx, const char *varname); > > int (*read_config)(struct uboot_ctx *ctx, const char *config); > > + int (*read_multi_config)(struct uboot_ctx **ctxlist, const char > *config); > > int (*load_file)(struct uboot_ctx *ctx, const char *filename); > > int (*set_env)(struct uboot_ctx *ctx, const char *varname, const > char *value); > > int (*env_store)(struct uboot_ctx *ctx); > > @@ -43,9 +44,12 @@ static int bootloader_initialize(struct uboot_ctx > **ctx) > > ERROR("Error: environment not initialized"); > > return -ENODEV; > > } > > - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { > > - ERROR("Configuration file %s wrong or corrupted", > CONFIG_UBOOT_FWENV); > > - return -EINVAL; > > + if (libuboot.read_multi_config(ctx, CONFIG_UBOOT_FWENV)) { > > + WARN("Configuration file %s not in new format, fallback to > legacy format", CONFIG_UBOOT_FWENV); > > + if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { > > + ERROR("Configuration file %s wrong or corrupted", > CONFIG_UBOOT_FWENV); > > + return -EINVAL; > > + } > > } > > if (libuboot.open(*ctx) < 0) { > > WARN("Cannot read environment, using default"); > > @@ -130,6 +134,7 @@ static bootloader* probe(void) > > libuboot.initialize = libuboot_initialize; > > libuboot.get_env = libuboot_get_env; > > libuboot.read_config = libuboot_read_config; > > + libuboot.read_multi_config = libuboot_read_multi_config; > > libuboot.load_file = libuboot_load_file; > > libuboot.set_env = libuboot_set_env; > > libuboot.env_store = libuboot_env_store; > > @@ -146,6 +151,7 @@ static bootloader* probe(void) > > load_symbol(handle, &libuboot.initialize, "libuboot_initialize"); > > load_symbol(handle, &libuboot.get_env, "libuboot_get_env"); > > load_symbol(handle, &libuboot.read_config, "libuboot_read_config"); > > + load_symbol(handle, &libuboot.read_multi_config, > "libuboot_read_multiple_config"); > > This does not work. It just work if libubootenv is pushed to the last > version incluiding support for YAML. If libubootenv does not support it, > load_symbol (macro) will close the handle and the library is closed. > This is unwanted. > > Because library is dynamically loaded, it is supposed that SWUpdate can > handle both, else an upgrade to SWUpdate will force an update to > libubootenv, even if the configuration format is still legacy. > I thought about it after making the modification, what do you think of modifying the read_config function in libubootenv so that it supports both formats as no modification will be necessary in swupdate? Best regards, Joris > > > load_symbol(handle, &libuboot.load_file, "libuboot_load_file"); > > load_symbol(handle, &libuboot.set_env, "libuboot_set_env"); > > load_symbol(handle, &libuboot.env_store, "libuboot_env_store"); > > Best regards, > Stefano > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ===================================================================== > >
Hi Joris, On 09.04.23 12:44, Joris OFFOUGA wrote: > > Hi Stefano, > > Le dim. 9 avr. 2023 à 12:25, Stefano Babic <sbabic@denx.de > <mailto:sbabic@denx.de>> a écrit : > > Hi Joris, > > On 08.04.23 22:39, Joris Offouga wrote: > > see > https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c <https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c> > > > > Signed-off-by: Joris Offouga <offougajoris@gmail.com > <mailto:offougajoris@gmail.com>> > > --- > > bootloader/uboot.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/bootloader/uboot.c b/bootloader/uboot.c > > index a2f0794..e3a307f 100644 > > --- a/bootloader/uboot.c > > +++ b/bootloader/uboot.c > > @@ -32,6 +32,7 @@ static struct { > > int (*initialize)(struct uboot_ctx **out, struct > uboot_env_device *envdevs); > > char* (*get_env)(struct uboot_ctx *ctx, const char *varname); > > int (*read_config)(struct uboot_ctx *ctx, const char > *config); > > + int (*read_multi_config)(struct uboot_ctx **ctxlist, > const char *config); > > int (*load_file)(struct uboot_ctx *ctx, const char > *filename); > > int (*set_env)(struct uboot_ctx *ctx, const char > *varname, const char *value); > > int (*env_store)(struct uboot_ctx *ctx); > > @@ -43,9 +44,12 @@ static int bootloader_initialize(struct > uboot_ctx **ctx) > > ERROR("Error: environment not initialized"); > > return -ENODEV; > > } > > - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { > > - ERROR("Configuration file %s wrong or corrupted", > CONFIG_UBOOT_FWENV); > > - return -EINVAL; > > + if (libuboot.read_multi_config(ctx, CONFIG_UBOOT_FWENV)) { > > + WARN("Configuration file %s not in new format, > fallback to legacy format", CONFIG_UBOOT_FWENV); > > + if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) > < 0) { > > + ERROR("Configuration file %s wrong or > corrupted", CONFIG_UBOOT_FWENV); > > + return -EINVAL; > > + } > > } > > if (libuboot.open(*ctx) < 0) { > > WARN("Cannot read environment, using default"); > > @@ -130,6 +134,7 @@ static bootloader* probe(void) > > libuboot.initialize = libuboot_initialize; > > libuboot.get_env = libuboot_get_env; > > libuboot.read_config = libuboot_read_config; > > + libuboot.read_multi_config = libuboot_read_multi_config; > > libuboot.load_file = libuboot_load_file; > > libuboot.set_env = libuboot_set_env; > > libuboot.env_store = libuboot_env_store; > > @@ -146,6 +151,7 @@ static bootloader* probe(void) > > load_symbol(handle, &libuboot.initialize, > "libuboot_initialize"); > > load_symbol(handle, &libuboot.get_env, "libuboot_get_env"); > > load_symbol(handle, &libuboot.read_config, > "libuboot_read_config"); > > + load_symbol(handle, &libuboot.read_multi_config, > "libuboot_read_multiple_config"); > > This does not work. It just work if libubootenv is pushed to the last > version incluiding support for YAML. If libubootenv does not support > it, > load_symbol (macro) will close the handle and the library is closed. > This is unwanted. > > Because library is dynamically loaded, it is supposed that SWUpdate can > handle both, else an upgrade to SWUpdate will force an update to > libubootenv, even if the configuration format is still legacy. > > I thought about it after making the modification, what do you think of > modifying the read_config function in > libubootenv so that it supports both formats as no modification will be > necessary in swupdate? There is a fingerprint issue that it raised when I extended it to YAMl, and I did not know how to clean solve. Maybe you have an idea :-). libuboot_read_config() and libuboot_read_multiple_config() have two different API. In fact, libuboot_read_multiple_config() returns an array of context (struct uboot_ctx **ctxlist), because it is now possible to have multiple sets of environment in a single YAML configuration file, that I called "namespaces". The legacy environment is strict bound to u-boot, and its has just one ctx (struct uboot_ctx *ctx), allocated via libuboot_initialize() - there was no reason before to have multiple contexts. The logical way today is to have a single libuboot_read_config(struct uboot_ctx **ctxlist, const char *config) , that handles internally the two formats. But because the library is not only used by SWUpdate and fw_printenv / fw_setenv, but there also many users linking the library to own applicatio, this raises a compatibility issue. Best regards, Stefano
Hi Joris, On 09.04.23 17:40, Stefano Babic wrote: > Hi Joris, > > On 09.04.23 12:44, Joris OFFOUGA wrote: >> >> Hi Stefano, >> >> Le dim. 9 avr. 2023 à 12:25, Stefano Babic <sbabic@denx.de >> <mailto:sbabic@denx.de>> a écrit : >> >> Hi Joris, >> >> On 08.04.23 22:39, Joris Offouga wrote: >> > see >> >> https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c <https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c> >> > >> > Signed-off-by: Joris Offouga <offougajoris@gmail.com >> <mailto:offougajoris@gmail.com>> >> > --- >> > bootloader/uboot.c | 12 +++++++++--- >> > 1 file changed, 9 insertions(+), 3 deletions(-) >> > >> > diff --git a/bootloader/uboot.c b/bootloader/uboot.c >> > index a2f0794..e3a307f 100644 >> > --- a/bootloader/uboot.c >> > +++ b/bootloader/uboot.c >> > @@ -32,6 +32,7 @@ static struct { >> > int (*initialize)(struct uboot_ctx **out, struct >> uboot_env_device *envdevs); >> > char* (*get_env)(struct uboot_ctx *ctx, const char >> *varname); >> > int (*read_config)(struct uboot_ctx *ctx, const char >> *config); >> > + int (*read_multi_config)(struct uboot_ctx **ctxlist, >> const char *config); >> > int (*load_file)(struct uboot_ctx *ctx, const char >> *filename); >> > int (*set_env)(struct uboot_ctx *ctx, const char >> *varname, const char *value); >> > int (*env_store)(struct uboot_ctx *ctx); >> > @@ -43,9 +44,12 @@ static int bootloader_initialize(struct >> uboot_ctx **ctx) >> > ERROR("Error: environment not initialized"); >> > return -ENODEV; >> > } >> > - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { >> > - ERROR("Configuration file %s wrong or corrupted", >> CONFIG_UBOOT_FWENV); >> > - return -EINVAL; >> > + if (libuboot.read_multi_config(ctx, CONFIG_UBOOT_FWENV)) { >> > + WARN("Configuration file %s not in new format, >> fallback to legacy format", CONFIG_UBOOT_FWENV); >> > + if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) >> < 0) { >> > + ERROR("Configuration file %s wrong or >> corrupted", CONFIG_UBOOT_FWENV); >> > + return -EINVAL; >> > + } >> > } >> > if (libuboot.open(*ctx) < 0) { >> > WARN("Cannot read environment, using default"); >> > @@ -130,6 +134,7 @@ static bootloader* probe(void) >> > libuboot.initialize = libuboot_initialize; >> > libuboot.get_env = libuboot_get_env; >> > libuboot.read_config = libuboot_read_config; >> > + libuboot.read_multi_config = libuboot_read_multi_config; >> > libuboot.load_file = libuboot_load_file; >> > libuboot.set_env = libuboot_set_env; >> > libuboot.env_store = libuboot_env_store; >> > @@ -146,6 +151,7 @@ static bootloader* probe(void) >> > load_symbol(handle, &libuboot.initialize, >> "libuboot_initialize"); >> > load_symbol(handle, &libuboot.get_env, "libuboot_get_env"); >> > load_symbol(handle, &libuboot.read_config, >> "libuboot_read_config"); >> > + load_symbol(handle, &libuboot.read_multi_config, >> "libuboot_read_multiple_config"); >> >> This does not work. It just work if libubootenv is pushed to the last >> version incluiding support for YAML. If libubootenv does not support >> it, >> load_symbol (macro) will close the handle and the library is closed. >> This is unwanted. >> >> Because library is dynamically loaded, it is supposed that >> SWUpdate can >> handle both, else an upgrade to SWUpdate will force an update to >> libubootenv, even if the configuration format is still legacy. >> >> I thought about it after making the modification, what do you think of >> modifying the read_config function in >> libubootenv so that it supports both formats as no modification will >> be necessary in swupdate? > > There is a fingerprint issue that it raised when I extended it to YAMl, > and I did not know how to clean solve. Maybe you have an idea :-). > > libuboot_read_config() and libuboot_read_multiple_config() have two > different API. In fact, libuboot_read_multiple_config() returns an array > of context (struct uboot_ctx **ctxlist), because it is now possible to > have multiple sets of environment in a single YAML configuration file, > that I called "namespaces". > > The legacy environment is strict bound to u-boot, and its has just one > ctx (struct uboot_ctx *ctx), allocated via libuboot_initialize() - there > was no reason before to have multiple contexts. > > The logical way today is to have a single libuboot_read_config(struct > uboot_ctx **ctxlist, const char *config) , that handles internally the > two formats. But because the library is not only used by SWUpdate and > fw_printenv / fw_setenv, but there also many users linking the library > to own applicatio, this raises a compatibility issue. > I try to address this, see my post for libubootenv. API is compatible with past, newer application just calls libuboot_read_config_ext() independently from the configuration file type. Change in SWUpdate is still required to switch to newer API. Best regards, Stefano
Hi Stefano, I see your patch and I'm preparing a build to test it with my project [1] , so this call should be removed [2] ? [1]: https://github.com/bdx-iot/meta-swupdate-dev/tree/topic/update/layer [2]: https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L42 Le lun. 10 avr. 2023 à 12:02, Stefano Babic <sbabic@denx.de> a écrit : > Hi Joris, > > On 09.04.23 17:40, Stefano Babic wrote: > > Hi Joris, > > > > On 09.04.23 12:44, Joris OFFOUGA wrote: > >> > >> Hi Stefano, > >> > >> Le dim. 9 avr. 2023 à 12:25, Stefano Babic <sbabic@denx.de > >> <mailto:sbabic@denx.de>> a écrit : > >> > >> Hi Joris, > >> > >> On 08.04.23 22:39, Joris Offouga wrote: > >> > see > >> > >> > https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c > < > https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c > > > >> > > >> > Signed-off-by: Joris Offouga <offougajoris@gmail.com > >> <mailto:offougajoris@gmail.com>> > >> > --- > >> > bootloader/uboot.c | 12 +++++++++--- > >> > 1 file changed, 9 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/bootloader/uboot.c b/bootloader/uboot.c > >> > index a2f0794..e3a307f 100644 > >> > --- a/bootloader/uboot.c > >> > +++ b/bootloader/uboot.c > >> > @@ -32,6 +32,7 @@ static struct { > >> > int (*initialize)(struct uboot_ctx **out, struct > >> uboot_env_device *envdevs); > >> > char* (*get_env)(struct uboot_ctx *ctx, const char > >> *varname); > >> > int (*read_config)(struct uboot_ctx *ctx, const char > >> *config); > >> > + int (*read_multi_config)(struct uboot_ctx **ctxlist, > >> const char *config); > >> > int (*load_file)(struct uboot_ctx *ctx, const char > >> *filename); > >> > int (*set_env)(struct uboot_ctx *ctx, const char > >> *varname, const char *value); > >> > int (*env_store)(struct uboot_ctx *ctx); > >> > @@ -43,9 +44,12 @@ static int bootloader_initialize(struct > >> uboot_ctx **ctx) > >> > ERROR("Error: environment not initialized"); > >> > return -ENODEV; > >> > } > >> > - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { > >> > - ERROR("Configuration file %s wrong or corrupted", > >> CONFIG_UBOOT_FWENV); > >> > - return -EINVAL; > >> > + if (libuboot.read_multi_config(ctx, CONFIG_UBOOT_FWENV)) { > >> > + WARN("Configuration file %s not in new format, > >> fallback to legacy format", CONFIG_UBOOT_FWENV); > >> > + if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) > >> < 0) { > >> > + ERROR("Configuration file %s wrong or > >> corrupted", CONFIG_UBOOT_FWENV); > >> > + return -EINVAL; > >> > + } > >> > } > >> > if (libuboot.open(*ctx) < 0) { > >> > WARN("Cannot read environment, using default"); > >> > @@ -130,6 +134,7 @@ static bootloader* probe(void) > >> > libuboot.initialize = libuboot_initialize; > >> > libuboot.get_env = libuboot_get_env; > >> > libuboot.read_config = libuboot_read_config; > >> > + libuboot.read_multi_config = libuboot_read_multi_config; > >> > libuboot.load_file = libuboot_load_file; > >> > libuboot.set_env = libuboot_set_env; > >> > libuboot.env_store = libuboot_env_store; > >> > @@ -146,6 +151,7 @@ static bootloader* probe(void) > >> > load_symbol(handle, &libuboot.initialize, > >> "libuboot_initialize"); > >> > load_symbol(handle, &libuboot.get_env, "libuboot_get_env"); > >> > load_symbol(handle, &libuboot.read_config, > >> "libuboot_read_config"); > >> > + load_symbol(handle, &libuboot.read_multi_config, > >> "libuboot_read_multiple_config"); > >> > >> This does not work. It just work if libubootenv is pushed to the > last > >> version incluiding support for YAML. If libubootenv does not support > >> it, > >> load_symbol (macro) will close the handle and the library is closed. > >> This is unwanted. > >> > >> Because library is dynamically loaded, it is supposed that > >> SWUpdate can > >> handle both, else an upgrade to SWUpdate will force an update to > >> libubootenv, even if the configuration format is still legacy. > >> > >> I thought about it after making the modification, what do you think of > >> modifying the read_config function in > >> libubootenv so that it supports both formats as no modification will > >> be necessary in swupdate? > > > > There is a fingerprint issue that it raised when I extended it to YAMl, > > and I did not know how to clean solve. Maybe you have an idea :-). > > > > libuboot_read_config() and libuboot_read_multiple_config() have two > > different API. In fact, libuboot_read_multiple_config() returns an array > > of context (struct uboot_ctx **ctxlist), because it is now possible to > > have multiple sets of environment in a single YAML configuration file, > > that I called "namespaces". > > > > The legacy environment is strict bound to u-boot, and its has just one > > ctx (struct uboot_ctx *ctx), allocated via libuboot_initialize() - there > > was no reason before to have multiple contexts. > > > > The logical way today is to have a single libuboot_read_config(struct > > uboot_ctx **ctxlist, const char *config) , that handles internally the > > two formats. But because the library is not only used by SWUpdate and > > fw_printenv / fw_setenv, but there also many users linking the library > > to own applicatio, this raises a compatibility issue. > > > > I try to address this, see my post for libubootenv. API is compatible > with past, newer application just calls libuboot_read_config_ext() > independently from the configuration file type. Change in SWUpdate is > still required to switch to newer API. > > Best regards, > Stefano > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ===================================================================== > >
Hi Joris, On 10.04.23 14:36, Joris OFFOUGA wrote: > Hi Stefano, > > I see your patch and I'm preparing a build to test it with my project > [1] , so this call should be removed [2] ? > > [1]: > https://github.com/bdx-iot/meta-swupdate-dev/tree/topic/update/layer > <https://github.com/bdx-iot/meta-swupdate-dev/tree/topic/update/layer> > [2]: > https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L42 > <https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L42> > Yes, it becomes: static int bootloader_initialize(struct uboot_ctx **ctx) { if (libuboot.read_config_ext(ctx, CONFIG_UBOOT_FWENV) < 0) { However, if libubootenv can support application with old and new API, SWUpdate should check first if ead_config_ext is available. Best regards, Stefano Babic > Le lun. 10 avr. 2023 à 12:02, Stefano Babic <sbabic@denx.de > <mailto:sbabic@denx.de>> a écrit : > > Hi Joris, > > On 09.04.23 17:40, Stefano Babic wrote: > > Hi Joris, > > > > On 09.04.23 12:44, Joris OFFOUGA wrote: > >> > >> Hi Stefano, > >> > >> Le dim. 9 avr. 2023 à 12:25, Stefano Babic <sbabic@denx.de > <mailto:sbabic@denx.de> > >> <mailto:sbabic@denx.de <mailto:sbabic@denx.de>>> a écrit : > >> > >> Hi Joris, > >> > >> On 08.04.23 22:39, Joris Offouga wrote: > >> > see > >> > >> > https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c <https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c> <https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c <https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c>> > >> > > >> > Signed-off-by: Joris Offouga <offougajoris@gmail.com > <mailto:offougajoris@gmail.com> > >> <mailto:offougajoris@gmail.com <mailto:offougajoris@gmail.com>>> > >> > --- > >> > bootloader/uboot.c | 12 +++++++++--- > >> > 1 file changed, 9 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/bootloader/uboot.c b/bootloader/uboot.c > >> > index a2f0794..e3a307f 100644 > >> > --- a/bootloader/uboot.c > >> > +++ b/bootloader/uboot.c > >> > @@ -32,6 +32,7 @@ static struct { > >> > int (*initialize)(struct uboot_ctx **out, struct > >> uboot_env_device *envdevs); > >> > char* (*get_env)(struct uboot_ctx *ctx, const char > >> *varname); > >> > int (*read_config)(struct uboot_ctx *ctx, const char > >> *config); > >> > + int (*read_multi_config)(struct uboot_ctx **ctxlist, > >> const char *config); > >> > int (*load_file)(struct uboot_ctx *ctx, const char > >> *filename); > >> > int (*set_env)(struct uboot_ctx *ctx, const char > >> *varname, const char *value); > >> > int (*env_store)(struct uboot_ctx *ctx); > >> > @@ -43,9 +44,12 @@ static int bootloader_initialize(struct > >> uboot_ctx **ctx) > >> > ERROR("Error: environment not initialized"); > >> > return -ENODEV; > >> > } > >> > - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) > < 0) { > >> > - ERROR("Configuration file %s wrong or > corrupted", > >> CONFIG_UBOOT_FWENV); > >> > - return -EINVAL; > >> > + if (libuboot.read_multi_config(ctx, > CONFIG_UBOOT_FWENV)) { > >> > + WARN("Configuration file %s not in new format, > >> fallback to legacy format", CONFIG_UBOOT_FWENV); > >> > + if (libuboot.read_config(*ctx, > CONFIG_UBOOT_FWENV) > >> < 0) { > >> > + ERROR("Configuration file %s wrong or > >> corrupted", CONFIG_UBOOT_FWENV); > >> > + return -EINVAL; > >> > + } > >> > } > >> > if (libuboot.open(*ctx) < 0) { > >> > WARN("Cannot read environment, using default"); > >> > @@ -130,6 +134,7 @@ static bootloader* probe(void) > >> > libuboot.initialize = libuboot_initialize; > >> > libuboot.get_env = libuboot_get_env; > >> > libuboot.read_config = libuboot_read_config; > >> > + libuboot.read_multi_config = > libuboot_read_multi_config; > >> > libuboot.load_file = libuboot_load_file; > >> > libuboot.set_env = libuboot_set_env; > >> > libuboot.env_store = libuboot_env_store; > >> > @@ -146,6 +151,7 @@ static bootloader* probe(void) > >> > load_symbol(handle, &libuboot.initialize, > >> "libuboot_initialize"); > >> > load_symbol(handle, &libuboot.get_env, > "libuboot_get_env"); > >> > load_symbol(handle, &libuboot.read_config, > >> "libuboot_read_config"); > >> > + load_symbol(handle, &libuboot.read_multi_config, > >> "libuboot_read_multiple_config"); > >> > >> This does not work. It just work if libubootenv is pushed to > the last > >> version incluiding support for YAML. If libubootenv does not > support > >> it, > >> load_symbol (macro) will close the handle and the library is > closed. > >> This is unwanted. > >> > >> Because library is dynamically loaded, it is supposed that > >> SWUpdate can > >> handle both, else an upgrade to SWUpdate will force an update to > >> libubootenv, even if the configuration format is still legacy. > >> > >> I thought about it after making the modification, what do you > think of > >> modifying the read_config function in > >> libubootenv so that it supports both formats as no modification > will > >> be necessary in swupdate? > > > > There is a fingerprint issue that it raised when I extended it to > YAMl, > > and I did not know how to clean solve. Maybe you have an idea :-). > > > > libuboot_read_config() and libuboot_read_multiple_config() have two > > different API. In fact, libuboot_read_multiple_config() returns > an array > > of context (struct uboot_ctx **ctxlist), because it is now > possible to > > have multiple sets of environment in a single YAML configuration > file, > > that I called "namespaces". > > > > The legacy environment is strict bound to u-boot, and its has > just one > > ctx (struct uboot_ctx *ctx), allocated via libuboot_initialize() > - there > > was no reason before to have multiple contexts. > > > > The logical way today is to have a single > libuboot_read_config(struct > > uboot_ctx **ctxlist, const char *config) , that handles > internally the > > two formats. But because the library is not only used by SWUpdate > and > > fw_printenv / fw_setenv, but there also many users linking the > library > > to own applicatio, this raises a compatibility issue. > > > > I try to address this, see my post for libubootenv. API is compatible > with past, newer application just calls libuboot_read_config_ext() > independently from the configuration file type. Change in SWUpdate is > still required to switch to newer API. > > Best regards, > Stefano > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: > sbabic@denx.de <mailto:sbabic@denx.de> > ===================================================================== > > -- > 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/CACr73kBa5mpPMhSh8bvHmDknKeu7QXAgvg1xUfJha5fR6b8n-A%40mail.gmail.com <https://groups.google.com/d/msgid/swupdate/CACr73kBa5mpPMhSh8bvHmDknKeu7QXAgvg1xUfJha5fR6b8n-A%40mail.gmail.com?utm_medium=email&utm_source=footer>.
Hi Stefano Le lun. 10 avr. 2023 à 14:44, Stefano Babic <sbabic@denx.de> a écrit : > Hi Joris, > > On 10.04.23 14:36, Joris OFFOUGA wrote: > > Hi Stefano, > > > > I see your patch and I'm preparing a build to test it with my project > > [1] , so this call should be removed [2] ? > > > > [1]: > > https://github.com/bdx-iot/meta-swupdate-dev/tree/topic/update/layer > > <https://github.com/bdx-iot/meta-swupdate-dev/tree/topic/update/layer> > > [2]: > > https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L42 > > <https://github.com/sbabic/swupdate/blob/master/bootloader/uboot.c#L42> > > > > Yes, it becomes: > > static int bootloader_initialize(struct uboot_ctx **ctx) > { > if (libuboot.read_config_ext(ctx, CONFIG_UBOOT_FWENV) < 0) { > > However, if libubootenv can support application with old and new API, > SWUpdate should check first if ead_config_ext is available. > > Is it something like that? https://github.com/bdx-iot/swupdate/commit/552db24439000360ae6bdf0b9797dc4f00484380 But I think I'm missing a condition for BOOTLOADER_STATIC_LINKED, when the read_config_ext function is missing at compile time Best regards, > Stefano Babic > > > Le lun. 10 avr. 2023 à 12:02, Stefano Babic <sbabic@denx.de > > <mailto:sbabic@denx.de>> a écrit : > > > > Hi Joris, > > > > On 09.04.23 17:40, Stefano Babic wrote: > > > Hi Joris, > > > > > > On 09.04.23 12:44, Joris OFFOUGA wrote: > > >> > > >> Hi Stefano, > > >> > > >> Le dim. 9 avr. 2023 à 12:25, Stefano Babic <sbabic@denx.de > > <mailto:sbabic@denx.de> > > >> <mailto:sbabic@denx.de <mailto:sbabic@denx.de>>> a écrit : > > >> > > >> Hi Joris, > > >> > > >> On 08.04.23 22:39, Joris Offouga wrote: > > >> > see > > >> > > >> > > > https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c > < > https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c> > < > https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c > < > https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c > >> > > >> > > > >> > Signed-off-by: Joris Offouga <offougajoris@gmail.com > > <mailto:offougajoris@gmail.com> > > >> <mailto:offougajoris@gmail.com <mailto: > offougajoris@gmail.com>>> > > >> > --- > > >> > bootloader/uboot.c | 12 +++++++++--- > > >> > 1 file changed, 9 insertions(+), 3 deletions(-) > > >> > > > >> > diff --git a/bootloader/uboot.c b/bootloader/uboot.c > > >> > index a2f0794..e3a307f 100644 > > >> > --- a/bootloader/uboot.c > > >> > +++ b/bootloader/uboot.c > > >> > @@ -32,6 +32,7 @@ static struct { > > >> > int (*initialize)(struct uboot_ctx **out, struct > > >> uboot_env_device *envdevs); > > >> > char* (*get_env)(struct uboot_ctx *ctx, const char > > >> *varname); > > >> > int (*read_config)(struct uboot_ctx *ctx, const > char > > >> *config); > > >> > + int (*read_multi_config)(struct uboot_ctx > **ctxlist, > > >> const char *config); > > >> > int (*load_file)(struct uboot_ctx *ctx, const char > > >> *filename); > > >> > int (*set_env)(struct uboot_ctx *ctx, const char > > >> *varname, const char *value); > > >> > int (*env_store)(struct uboot_ctx *ctx); > > >> > @@ -43,9 +44,12 @@ static int bootloader_initialize(struct > > >> uboot_ctx **ctx) > > >> > ERROR("Error: environment not initialized"); > > >> > return -ENODEV; > > >> > } > > >> > - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) > > < 0) { > > >> > - ERROR("Configuration file %s wrong or > > corrupted", > > >> CONFIG_UBOOT_FWENV); > > >> > - return -EINVAL; > > >> > + if (libuboot.read_multi_config(ctx, > > CONFIG_UBOOT_FWENV)) { > > >> > + WARN("Configuration file %s not in new > format, > > >> fallback to legacy format", CONFIG_UBOOT_FWENV); > > >> > + if (libuboot.read_config(*ctx, > > CONFIG_UBOOT_FWENV) > > >> < 0) { > > >> > + ERROR("Configuration file %s wrong > or > > >> corrupted", CONFIG_UBOOT_FWENV); > > >> > + return -EINVAL; > > >> > + } > > >> > } > > >> > if (libuboot.open(*ctx) < 0) { > > >> > WARN("Cannot read environment, using > default"); > > >> > @@ -130,6 +134,7 @@ static bootloader* probe(void) > > >> > libuboot.initialize = libuboot_initialize; > > >> > libuboot.get_env = libuboot_get_env; > > >> > libuboot.read_config = libuboot_read_config; > > >> > + libuboot.read_multi_config = > > libuboot_read_multi_config; > > >> > libuboot.load_file = libuboot_load_file; > > >> > libuboot.set_env = libuboot_set_env; > > >> > libuboot.env_store = libuboot_env_store; > > >> > @@ -146,6 +151,7 @@ static bootloader* probe(void) > > >> > load_symbol(handle, &libuboot.initialize, > > >> "libuboot_initialize"); > > >> > load_symbol(handle, &libuboot.get_env, > > "libuboot_get_env"); > > >> > load_symbol(handle, &libuboot.read_config, > > >> "libuboot_read_config"); > > >> > + load_symbol(handle, &libuboot.read_multi_config, > > >> "libuboot_read_multiple_config"); > > >> > > >> This does not work. It just work if libubootenv is pushed to > > the last > > >> version incluiding support for YAML. If libubootenv does not > > support > > >> it, > > >> load_symbol (macro) will close the handle and the library is > > closed. > > >> This is unwanted. > > >> > > >> Because library is dynamically loaded, it is supposed that > > >> SWUpdate can > > >> handle both, else an upgrade to SWUpdate will force an > update to > > >> libubootenv, even if the configuration format is still > legacy. > > >> > > >> I thought about it after making the modification, what do you > > think of > > >> modifying the read_config function in > > >> libubootenv so that it supports both formats as no modification > > will > > >> be necessary in swupdate? > > > > > > There is a fingerprint issue that it raised when I extended it to > > YAMl, > > > and I did not know how to clean solve. Maybe you have an idea :-). > > > > > > libuboot_read_config() and libuboot_read_multiple_config() have > two > > > different API. In fact, libuboot_read_multiple_config() returns > > an array > > > of context (struct uboot_ctx **ctxlist), because it is now > > possible to > > > have multiple sets of environment in a single YAML configuration > > file, > > > that I called "namespaces". > > > > > > The legacy environment is strict bound to u-boot, and its has > > just one > > > ctx (struct uboot_ctx *ctx), allocated via libuboot_initialize() > > - there > > > was no reason before to have multiple contexts. > > > > > > The logical way today is to have a single > > libuboot_read_config(struct > > > uboot_ctx **ctxlist, const char *config) , that handles > > internally the > > > two formats. But because the library is not only used by SWUpdate > > and > > > fw_printenv / fw_setenv, but there also many users linking the > > library > > > to own applicatio, this raises a compatibility issue. > > > > > > > I try to address this, see my post for libubootenv. API is compatible > > with past, newer application just calls libuboot_read_config_ext() > > independently from the configuration file type. Change in SWUpdate is > > still required to switch to newer API. > > > > Best regards, > > Stefano > > > > -- > > ===================================================================== > > DENX Software Engineering GmbH, Managing Director: Erika Unter > > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: > > sbabic@denx.de <mailto:sbabic@denx.de> > > ===================================================================== > > > > -- > > 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/CACr73kBa5mpPMhSh8bvHmDknKeu7QXAgvg1xUfJha5fR6b8n-A%40mail.gmail.com > < > https://groups.google.com/d/msgid/swupdate/CACr73kBa5mpPMhSh8bvHmDknKeu7QXAgvg1xUfJha5fR6b8n-A%40mail.gmail.com?utm_medium=email&utm_source=footer > >. > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Erika Unter > HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ===================================================================== > >
diff --git a/bootloader/uboot.c b/bootloader/uboot.c index a2f0794..e3a307f 100644 --- a/bootloader/uboot.c +++ b/bootloader/uboot.c @@ -32,6 +32,7 @@ static struct { int (*initialize)(struct uboot_ctx **out, struct uboot_env_device *envdevs); char* (*get_env)(struct uboot_ctx *ctx, const char *varname); int (*read_config)(struct uboot_ctx *ctx, const char *config); + int (*read_multi_config)(struct uboot_ctx **ctxlist, const char *config); int (*load_file)(struct uboot_ctx *ctx, const char *filename); int (*set_env)(struct uboot_ctx *ctx, const char *varname, const char *value); int (*env_store)(struct uboot_ctx *ctx); @@ -43,9 +44,12 @@ static int bootloader_initialize(struct uboot_ctx **ctx) ERROR("Error: environment not initialized"); return -ENODEV; } - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { - ERROR("Configuration file %s wrong or corrupted", CONFIG_UBOOT_FWENV); - return -EINVAL; + if (libuboot.read_multi_config(ctx, CONFIG_UBOOT_FWENV)) { + WARN("Configuration file %s not in new format, fallback to legacy format", CONFIG_UBOOT_FWENV); + if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { + ERROR("Configuration file %s wrong or corrupted", CONFIG_UBOOT_FWENV); + return -EINVAL; + } } if (libuboot.open(*ctx) < 0) { WARN("Cannot read environment, using default"); @@ -130,6 +134,7 @@ static bootloader* probe(void) libuboot.initialize = libuboot_initialize; libuboot.get_env = libuboot_get_env; libuboot.read_config = libuboot_read_config; + libuboot.read_multi_config = libuboot_read_multi_config; libuboot.load_file = libuboot_load_file; libuboot.set_env = libuboot_set_env; libuboot.env_store = libuboot_env_store; @@ -146,6 +151,7 @@ static bootloader* probe(void) load_symbol(handle, &libuboot.initialize, "libuboot_initialize"); load_symbol(handle, &libuboot.get_env, "libuboot_get_env"); load_symbol(handle, &libuboot.read_config, "libuboot_read_config"); + load_symbol(handle, &libuboot.read_multi_config, "libuboot_read_multiple_config"); load_symbol(handle, &libuboot.load_file, "libuboot_load_file"); load_symbol(handle, &libuboot.set_env, "libuboot_set_env"); load_symbol(handle, &libuboot.env_store, "libuboot_env_store");
see https://github.com/sbabic/libubootenv/commit/c6784ab6fae482f7e9d6d1a959ab64f545fa1a8c Signed-off-by: Joris Offouga <offougajoris@gmail.com> --- bootloader/uboot.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)