[PATCH 1/2] Introduce --gen-swversions parameter

[Marcus Folkesson]

After a successful update, --gen-swversions generates a file with
the installed software and their versions.

The file is compatible with CONFIG_SW_VERSIONS_FILE and could replace
that file if the end user want swupdate to maintain that file entirely.

Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
---
core/installer.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
core/swupdate.c | 7 ++++-
include/swupdate.h | 1 +
3 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/core/installer.c b/core/installer.c
index 48c7fe5e..83122388 100644
--- a/core/installer.c
+++ b/core/installer.c
@@ -187,6 +187,24 @@ static int prepare_var_script(struct dict *dict, const char *script)
return 0;
}

+static int generate_swversions(struct swupdate_cfg *cfg)
+{
+ FILE *fp;
+ struct sw_version *swver;
+ struct swver *sw_ver_list = &cfg->installed_sw_list;
+
+ fp = fopen(cfg->output_swversions, "w");
+ if (!fp)
+ return -EACCES;
+
+ LIST_FOREACH(swver, sw_ver_list, next) {
+ fprintf(fp, "%s\t\t%s\n", swver->name, swver->version);
+ }
+ fclose(fp);
+
+ return 0;
+}
+
static int update_bootloader_env(struct swupdate_cfg *cfg, const char *script)
{
int ret = 0;
@@ -279,6 +297,41 @@ int install_single_image(struct img_type *img, bool dry_run)
return ret;
}

+static int update_installed_image_version(struct swver *sw_ver_list,
+ struct img_type *img)
+{
+ struct sw_version *swver;
+ struct sw_version *swcomp;
+
+ if (!sw_ver_list)
+ return false;
+
+ LIST_FOREACH(swver, sw_ver_list, next) {
+ /*
+ * If component is already installed, update the version
+ */
+ if (!strncmp(img->id.name, swver->name, sizeof(img->id.name))) {
+ strncpy(swver->version, img->id.version, sizeof(img->id.version));
+ return true;
+ }
+ }
+
+ /*
+ * No previous version of this component is installed. Create a new entry.
+ */
+ swcomp = (struct sw_version *)calloc(1, sizeof(struct sw_version));
+ if (!swcomp) {
+ ERROR("Could not create new version entry.");
+ return false;
+ }
+
+ strlcpy(swcomp->name, img->id.name, sizeof(swcomp->name));
+ strlcpy(swcomp->version, img->id.version, sizeof(swcomp->version));
+ LIST_INSERT_HEAD(sw_ver_list, swcomp, next);
+
+ return true;
+}
+
/*
* streamfd: file descriptor if it is required to extract
* images from the stream (update from file)
@@ -370,6 +423,8 @@ int install_images(struct swupdate_cfg *sw)

close(img->fdin);

+ update_installed_image_version(&sw->installed_sw_list, img);
+
if (dropimg)
free_image(img);

@@ -409,6 +464,15 @@ int install_images(struct swupdate_cfg *sw)

ret |= run_prepost_scripts(&sw->bootscripts, POSTINSTALL);

+ /*
+ * Should we generate a list with installed software?
+ */
+ if (strlen(sw->output_swversions)) {
+ if (!generate_swversions(sw)) {
+ ERROR("%s cannot be opened", sw->output_swversions);
+ }
+ }
+
return ret;
}

diff --git a/core/swupdate.c b/core/swupdate.c
index a421e888..5cfec3d6 100644
--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -111,6 +111,7 @@ static struct option long_options[] = {
{"no-state-marker", no_argument, NULL, 'm'},
{"no-transaction-marker", no_argument, NULL, 'M'},
{"output", required_argument, NULL, 'o'},
+ {"gen-swversions", required_argument, NULL, 's'},
{"preupdate", required_argument, NULL, 'P'},
{"postupdate", required_argument, NULL, 'p'},
{"select", required_argument, NULL, 'e'},
@@ -173,6 +174,7 @@ static void usage(char *programname)
" -M, --no-transaction-marker : disable setting bootloader transaction marker\n"
" -m, --no-state-marker : disable setting update state in bootloader\n"
" -o, --output <filename> : saves the incoming stream\n"
+ " -s, --gen-swversions <filename>: generate sw-versions file after successful installation\n"
" -v, --verbose : be verbose, set maximum loglevel\n"
" --version : print SWUpdate version and exit\n"
#ifdef CONFIG_HW_COMPATIBILITY
@@ -475,7 +477,7 @@ int main(int argc, char **argv)
#endif
memset(main_options, 0, sizeof(main_options));
memset(image_url, 0, sizeof(image_url));
- strcpy(main_options, "vhni:e:gq:l:Lcf:p:P:o:N:R:MmB:");
+ strcpy(main_options, "vhni:e:gq:l:Lcf:p:P:o:s:N:R:MmB:");
#ifdef CONFIG_MTD
strcat(main_options, "b:");
#endif
@@ -623,6 +625,9 @@ int main(int argc, char **argv)
case 'o':
strlcpy(swcfg.output, optarg, sizeof(swcfg.output));
break;
+ case 's':
+ strlcpy(swcfg.output_swversions, optarg, sizeof(swcfg.output_swversions));
+ break;
case 'B':
if (set_bootloader(optarg) != 0) {
ERROR("Bootloader interface '%s' could not be initialized.", optarg);
diff --git a/include/swupdate.h b/include/swupdate.h
index e18de8d3..ecad2d82 100644
--- a/include/swupdate.h
+++ b/include/swupdate.h
@@ -49,6 +49,7 @@ struct swupdate_cfg {
bool bootloader_transaction_marker;
bool bootloader_state_marker;
char output[SWUPDATE_GENERAL_STRING_SIZE];
+ char output_swversions[SWUPDATE_GENERAL_STRING_SIZE];
char publickeyfname[SWUPDATE_GENERAL_STRING_SIZE];
char aeskeyfname[SWUPDATE_GENERAL_STRING_SIZE];
char postupdatecmd[SWUPDATE_GENERAL_STRING_SIZE];
--
2.44.0

[Marcus Folkesson]

Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
---

doc/source/swupdate.rst | 2 ++
1 file changed, 2 insertions(+)

diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
index ff00ca02..b8df7fea 100644
--- a/doc/source/swupdate.rst
+++ b/doc/source/swupdate.rst
@@ -538,6 +538,8 @@ Command line parameters
+-------------+----------+--------------------------------------------+
| -o <file> | string | Save the stream (SWU) to a file. |
+-------------+----------+--------------------------------------------+
+| -s <file> | string | Save installed version info to a file. |
++-------------+----------+--------------------------------------------+
| -v | - | Activate verbose output. |
+-------------+----------+--------------------------------------------+
| -M | - | Disable setting the bootloader transaction |
--
2.44.0

[Marcus Folkesson]

After a successful update, --gen-swversions generates a file with
the installed software and their versions.

The file is compatible with CONFIG_SW_VERSIONS_FILE and could replace
that file if the end user want swupdate to maintain that file entirely.

Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
---

Notes:
v2:
- reverse logic for:
if (generate_swversions(sw)) {

ERROR("%s cannot be opened", sw->output_swversions);
}

core/installer.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++
core/swupdate.c | 7 ++++-
include/swupdate.h | 1 +
3 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/core/installer.c b/core/installer.c

index 48c7fe5e..bbcc0497 100644

+ if (generate_swversions(sw)) {

[Marcus Folkesson]

Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
---

[Stefano Babic]

Hi Marcus,

On 19.04.24 10:48, Marcus Folkesson wrote:
> After a successful update, --gen-swversions generates a file with
> the installed software and their versions.
>
> The file is compatible with CONFIG_SW_VERSIONS_FILE and could replace
> that file if the end user want swupdate to maintain that file entirely.
>

This is a new feature and to be introduced I like to check pros and
cons. Currently, SWUpdate does not maintain /etc/sw-versions and this
should be provided by the system. The pattern is that device detects the
versions by booting and generates sw-versions.

The use case here is that versions are passed into sw-description, and
values are stored persistently. However, "version" attribute is not
mandatory and it is set when one of the flags for comparison
(installed-if-different,..) is set, too.

My concern is how the methods combine and work together, and what
happens if sw-description is not fully populated, that means version for
some artifacts are missing. I guess it is enough to document this,
because next time when an update runs, the same artifact will be
installed in any case.

The biggest concern is what is happening if writing the new sw-versions
is failing. The update was successful, but what is happening the next
time if your update is depending on it ?

Think this case:

- an artifact is currently unversioned. There is no entry in
/etc/sw-versions, that is no entry in sw_ver_list.
- you want to version it in the future, that means img->id.version is
set to be put in /etc/sw-versions for the next time.

But this code ignores it. It just works if the artifact *is* and *will
be* populated.

An ERROR in SWUpdate stops the update, this should scaled down at least
to WARN.

Runtime configuration is provided via command line parameter and/or
configuration file. It is up to the user to choose which one, and the
rule is that command line parms can overwrite swupdate.cfg. A new
configuration should also be supported into swupdate.cfg.

Best regards,
Stefano Babic

[Marcus Folkesson]

Hi Stefano,

First of all, thank you for your reply.

On Fri, Apr 19, 2024 at 11:57:32AM +0200, Stefano Babic wrote:
> Hi Marcus,
>
> On 19.04.24 10:48, Marcus Folkesson wrote:
> > After a successful update, --gen-swversions generates a file with
> > the installed software and their versions.
> >
> > The file is compatible with CONFIG_SW_VERSIONS_FILE and could replace
> > that file if the end user want swupdate to maintain that file entirely.
> >
>
> This is a new feature and to be introduced I like to check pros and
> cons. Currently, SWUpdate does not maintain /etc/sw-versions and this
> should be provided by the system. The pattern is that device detects the
> versions by booting and generates sw-versions.

Yes, I'm aware of that.
Even if that is the feature I actually strive for, I tried to pick a
name for the feature that more sounds like "hey, please generate a compilation
of the installed software, both old and newly installed and save it to
this file" rather than "Do my work and maintain sw-versions for me".

That is also why it is possible to pick a file that does not have to be the
CONFIG_SW_VERSIONS_FILE.

>
> The use case here is that versions are passed into sw-description, and
> values are stored persistently. However, "version" attribute is not
> mandatory and it is set when one of the flags for comparison
> (installed-if-different,..) is set, too.
>
> My concern is how the methods combine and work together, and what
> happens if sw-description is not fully populated, that means version for
> some artifacts are missing. I guess it is enough to document this,
> because next time when an update runs, the same artifact will be
> installed in any case.

I guess so, if no versions is populated then there is no different
with/without this feature.

>
> The biggest concern is what is happening if writing the new sw-versions
> is failing. The update was successful, but what is happening the next
> time if your update is depending on it ?

I fully understand your concern.

Hum hum, not sure if I get you.
Does not you talk about...

>
>
> > + }
> > + }
> > +
> > + /*
> > + * No previous version of this component is installed. Create a new entry.
> > + */
> > + swcomp = (struct sw_version *)calloc(1, sizeof(struct sw_version));
> > + if (!swcomp) {
> > + ERROR("Could not create new version entry.");
> > + return false;
> > + }

... this case?


But I should check if img->id.version is valid before doing this though.

Got it, thank you.

>
> > + }
> > + }
> > +
> > return ret;
> > }
> >
> > diff --git a/core/swupdate.c b/core/swupdate.c
> > index a421e888..5cfec3d6 100644
> > --- a/core/swupdate.c
> > +++ b/core/swupdate.c
> > @@ -111,6 +111,7 @@ static struct option long_options[] = {
> > {"no-state-marker", no_argument, NULL, 'm'},
> > {"no-transaction-marker", no_argument, NULL, 'M'},
> > {"output", required_argument, NULL, 'o'},
> > + {"gen-swversions", required_argument, NULL, 's'},

I could not come up with a better name than "gen-swversions", I'm not
totally happy with it and do not mind to change it if there are any
better suggestions.. just sayin :-)

Got it, thank you.

>
> Best regards,
> Stefano Babic

[Stefano Babic]

Hi Markus,

Artifacts already versioned are in /etc/sw-versions, that is in this
code they are in swver list. The use case I think is if an artifact was
not yet versioned (missing entry in swver), but you plan to version in
future. A new entry should go into sw-version, but the code above does
not cover this case because img->id.name cannot be found in the list
(and then it is skipped).>>

>>
>>> + }
>>> + }
>>> +
>>> + /*
>>> + * No previous version of this component is installed. Create a new entry.
>>> + */
>>> + swcomp = (struct sw_version *)calloc(1, sizeof(struct sw_version));
>>> + if (!swcomp) {
>>> + ERROR("Could not create new version entry.");
>>> + return false;
>>> + }
>
> ... this case?
>
>
> But I should check if img->id.version is valid before doing this though.
>

Right

Name is ok, but you have just implemented the configuration via comman
line parameter (-s) and not from configuration file (swupdate.cfg).

Best regards,
Stefano

[Marcus Folkesson]

After a successful update, --gen-swversions generates a file with
the installed software and their versions.

The file is compatible with CONFIG_SW_VERSIONS_FILE and could replace
that file if the end user want swupdate to maintain that file entirely.

Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
---

Notes:
v2:
- reverse logic for:
if (generate_swversions(sw)) {
ERROR("%s cannot be opened", sw->output_swversions);
}

v3:
- Change ERROR to WARN
- Handle special cases in update_installed_image_version()
- Add gen-swversions config parameter

core/installer.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++
core/swupdate.c | 9 ++++++-
include/swupdate.h | 1 +
3 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/core/installer.c b/core/installer.c
index 48c7fe5e..c0ea1555 100644

--- a/core/installer.c
+++ b/core/installer.c
@@ -187,6 +187,24 @@ static int prepare_var_script(struct dict *dict, const char *script)
return 0;
}

+static int generate_swversions(struct swupdate_cfg *cfg)
+{
+ FILE *fp;
+ struct sw_version *swver;
+ struct swver *sw_ver_list = &cfg->installed_sw_list;
+
+ fp = fopen(cfg->output_swversions, "w");
+ if (!fp)
+ return -EACCES;
+
+ LIST_FOREACH(swver, sw_ver_list, next) {
+ fprintf(fp, "%s\t\t%s\n", swver->name, swver->version);
+ }
+ fclose(fp);
+
+ return 0;
+}
+
static int update_bootloader_env(struct swupdate_cfg *cfg, const char *script)
{
int ret = 0;

@@ -279,6 +297,43 @@ int install_single_image(struct img_type *img, bool dry_run)

return ret;
}

+static int update_installed_image_version(struct swver *sw_ver_list,
+ struct img_type *img)
+{
+ struct sw_version *swver;
+ struct sw_version *swcomp;
+

+ if (sw_ver_list) {

+ LIST_FOREACH(swver, sw_ver_list, next) {
+ /*
+ * If component is already installed, update the version
+ */
+ if (!strncmp(img->id.name, swver->name, sizeof(img->id.name))) {
+ strncpy(swver->version, img->id.version, sizeof(img->id.version));
+ return true;

+ }
+ }
+ }
+
+ if (!strlen(img->id.version))
+ return false;
+

+ /*
+ * No previous version of this component is installed. Create a new entry.
+ */
+ swcomp = (struct sw_version *)calloc(1, sizeof(struct sw_version));
+ if (!swcomp) {
+ ERROR("Could not create new version entry.");
+ return false;
+ }

+
+ strlcpy(swcomp->name, img->id.name, sizeof(swcomp->name));
+ strlcpy(swcomp->version, img->id.version, sizeof(swcomp->version));
+ LIST_INSERT_HEAD(sw_ver_list, swcomp, next);
+
+ return true;
+}
+
/*
* streamfd: file descriptor if it is required to extract
* images from the stream (update from file)

@@ -370,6 +425,8 @@ int install_images(struct swupdate_cfg *sw)

close(img->fdin);

+ update_installed_image_version(&sw->installed_sw_list, img);
+
if (dropimg)
free_image(img);

@@ -409,6 +466,15 @@ int install_images(struct swupdate_cfg *sw)

ret |= run_prepost_scripts(&sw->bootscripts, POSTINSTALL);

+ /*
+ * Should we generate a list with installed software?
+ */
+ if (strlen(sw->output_swversions)) {
+ if (generate_swversions(sw)) {

+ WARN("%s cannot be opened", sw->output_swversions);

+ }
+ }
+
return ret;
}

diff --git a/core/swupdate.c b/core/swupdate.c

index a421e888..6ddff310 100644

--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -111,6 +111,7 @@ static struct option long_options[] = {
{"no-state-marker", no_argument, NULL, 'm'},
{"no-transaction-marker", no_argument, NULL, 'M'},
{"output", required_argument, NULL, 'o'},
+ {"gen-swversions", required_argument, NULL, 's'},

{"preupdate", required_argument, NULL, 'P'},
{"postupdate", required_argument, NULL, 'p'},
{"select", required_argument, NULL, 'e'},
@@ -173,6 +174,7 @@ static void usage(char *programname)
" -M, --no-transaction-marker : disable setting bootloader transaction marker\n"
" -m, --no-state-marker : disable setting update state in bootloader\n"
" -o, --output <filename> : saves the incoming stream\n"
+ " -s, --gen-swversions <filename>: generate sw-versions file after successful installation\n"
" -v, --verbose : be verbose, set maximum loglevel\n"
" --version : print SWUpdate version and exit\n"
#ifdef CONFIG_HW_COMPATIBILITY

@@ -320,6 +322,8 @@ static int read_globals_settings(void *elem, void *data)
"preupdatecmd", sw->preupdatecmd);
GET_FIELD_STRING(LIBCFG_PARSER, elem,
"namespace-vars", sw->namespace_for_vars);
+ GET_FIELD_STRING(LIBCFG_PARSER, elem,
+ "gen-swversions", sw->output_swversions);
if (strlen(sw->namespace_for_vars)) {
if (!swupdate_set_default_namespace(sw->namespace_for_vars))
WARN("Default Namaspace for SWUpdate vars cannot be set, possible side-effects");
@@ -475,7 +479,7 @@ int main(int argc, char **argv)

#endif
memset(main_options, 0, sizeof(main_options));
memset(image_url, 0, sizeof(image_url));
- strcpy(main_options, "vhni:e:gq:l:Lcf:p:P:o:N:R:MmB:");
+ strcpy(main_options, "vhni:e:gq:l:Lcf:p:P:o:s:N:R:MmB:");
#ifdef CONFIG_MTD
strcat(main_options, "b:");
#endif

@@ -623,6 +627,9 @@ int main(int argc, char **argv)

case 'o':
strlcpy(swcfg.output, optarg, sizeof(swcfg.output));
break;
+ case 's':
+ strlcpy(swcfg.output_swversions, optarg, sizeof(swcfg.output_swversions));
+ break;
case 'B':
if (set_bootloader(optarg) != 0) {
ERROR("Bootloader interface '%s' could not be initialized.", optarg);
diff --git a/include/swupdate.h b/include/swupdate.h
index e18de8d3..ecad2d82 100644
--- a/include/swupdate.h
+++ b/include/swupdate.h
@@ -49,6 +49,7 @@ struct swupdate_cfg {
bool bootloader_transaction_marker;
bool bootloader_state_marker;
char output[SWUPDATE_GENERAL_STRING_SIZE];
+ char output_swversions[SWUPDATE_GENERAL_STRING_SIZE];
char publickeyfname[SWUPDATE_GENERAL_STRING_SIZE];
char aeskeyfname[SWUPDATE_GENERAL_STRING_SIZE];
char postupdatecmd[SWUPDATE_GENERAL_STRING_SIZE];

--
2.44.0

[Marcus Folkesson]

Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
---

doc/source/swupdate.rst | 2 ++
examples/configuration/swupdate.cfg | 1 +
2 files changed, 3 insertions(+)

diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst
index ff00ca02..b8df7fea 100644
--- a/doc/source/swupdate.rst
+++ b/doc/source/swupdate.rst
@@ -538,6 +538,8 @@ Command line parameters
+-------------+----------+--------------------------------------------+
| -o <file> | string | Save the stream (SWU) to a file. |
+-------------+----------+--------------------------------------------+
+| -s <file> | string | Save installed version info to a file. |
++-------------+----------+--------------------------------------------+
| -v | - | Activate verbose output. |
+-------------+----------+--------------------------------------------+
| -M | - | Disable setting the bootloader transaction |

diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg
index b46c92d9..69a57e61 100644
--- a/examples/configuration/swupdate.cfg
+++ b/examples/configuration/swupdate.cfg
@@ -52,6 +52,7 @@
# variables. This can be overridden in sw-description.
# It is one set in libubootenv configuration file.
# fwenv-config-location : path of the configuration file for libubootenv
+# gen-swversions : generate a version file containing all installed (versioned) images.
globals :
{

--
2.44.0

[Dominique MARTINET]

Stefano Babic wrote on Fri, Apr 19, 2024 at 11:57:32AM +0200:
> The biggest concern is what is happening if writing the new sw-versions
> is failing. The update was successful, but what is happening the next
> time if your update is depending on it ?

For reference I am doing something similar, and made writing sw-versions
mandatory as part of the last step of the update e.g. if that fails then
the update isn't applied and the system keeps running the old version.

Perhaps something similar could be implemented here, as I'd expect the
boot partiton switch/reboot logic to be after this anyway.


(I probably wouldn't use this for the forseeable future anyway given my
update scripts need to be backwards compatible with previous swupdate
versions anyway, so I'd rather keep using the "update sw-versions" logic
I provide to ensure it doesn't regress more easily, but I think it's a
valid pattern even if I'd tend to side with the "generate sw-versions on
boot" from an integrator point of view)

--
Dominique

[Stefano Babic]

Hi Dominique, Markus,

On 22.04.24 06:10, Dominique MARTINET wrote:
> Stefano Babic wrote on Fri, Apr 19, 2024 at 11:57:32AM +0200:
>> The biggest concern is what is happening if writing the new sw-versions
>> is failing. The update was successful, but what is happening the next
>> time if your update is depending on it ?
>
> For reference I am doing something similar, and made writing sw-versions
> mandatory as part of the last step of the update e.g. if that fails then
> the update isn't applied and the system keeps running the old version.
>

This is my concern, too. If sw-versions is part of the update, its
generation must be successful, or the update fails.

Markus' patches introduces this feature as part of the core, and then I
think it must be coherent. Patches creates a sort of "post command",
that is executed, but its result is not relevant for the update. The
command is always executed, independently from the update / SWU.

However, I think that the result is relevant and if it fails, old
version should be retained. Instead of having some script like Dominique
is doing, a more cleaner way should be to wrap Markus' functions into an
own handler. The big advantage I see is that we do not need to restart
SWUpdate if we do not need for a single update to store sw-versions,
that is this feature could be enabled at SWU level.

So my suggestion is to introduce a small handler with mask
SCRIPT_HANDLER (just as postinstall) and the output file for
/etc/sw-version can be set in one of the properties. In this way, this
feature can be enabled or disabled on a SWU basis.

> Perhaps something similar could be implemented here, as I'd expect the
> boot partiton switch/reboot logic to be after this anyway.

This is not what I expect - if writing /etc/sw-version is important, it
should be done and its result cannot be ignored. The logic is already in
place if cod eis moved into an own handler because switch/reboot is done
as last step.

>
>
> (I probably wouldn't use this for the forseeable future anyway given my
> update scripts need to be backwards compatible with previous swupdate
> versions anyway, so I'd rather keep using the "update sw-versions" logic
> I provide to ensure it doesn't regress more easily, but I think it's a
> valid pattern even if I'd tend to side with the "generate sw-versions on
> boot" from an integrator point of view)

Sure, but Markus raises the issue and this can be a new feature that can
be used or not, and this is easier if it is managed by a separate
handler instead of core code like in the patches.

Best regards,
Stefano

[Marcus Folkesson]

Hello Stefano,

On Mon, Apr 22, 2024 at 09:35:47AM +0200, Stefano Babic wrote:
> Hi Dominique, Markus,
>
> On 22.04.24 06:10, Dominique MARTINET wrote:
> > Stefano Babic wrote on Fri, Apr 19, 2024 at 11:57:32AM +0200:
> > > The biggest concern is what is happening if writing the new sw-versions
> > > is failing. The update was successful, but what is happening the next
> > > time if your update is depending on it ?
> >
> > For reference I am doing something similar, and made writing sw-versions
> > mandatory as part of the last step of the update e.g. if that fails then
> > the update isn't applied and the system keeps running the old version.
> >
>
> This is my concern, too. If sw-versions is part of the update, its
> generation must be successful, or the update fails.
>
> Markus' patches introduces this feature as part of the core, and then I
> think it must be coherent. Patches creates a sort of "post command",
> that is executed, but its result is not relevant for the update. The
> command is always executed, independently from the update / SWU.
>
> However, I think that the result is relevant and if it fails, old
> version should be retained. Instead of having some script like Dominique

I agree with you.

> is doing, a more cleaner way should be to wrap Markus' functions into an
> own handler. The big advantage I see is that we do not need to restart
> SWUpdate if we do not need for a single update to store sw-versions,
> that is this feature could be enabled at SWU level.

Is it a feature that we would like to control on a SWU level?
I think that using this feature or not is more of a system design
decision and should be part of the configuration file.

But that is not a strong opinion and I can work with either solution.

>
> So my suggestion is to introduce a small handler with mask
> SCRIPT_HANDLER (just as postinstall) and the output file for
> /etc/sw-version can be set in one of the properties. In this way, this
> feature can be enabled or disabled on a SWU basis.

Looking into the code, I do not think the context of a handler has
everything needed (basically the installed_sw_list) to do this, or do I
missinterpret?

>
> > Perhaps something similar could be implemented here, as I'd expect the
> > boot partiton switch/reboot logic to be after this anyway.
>
> This is not what I expect - if writing /etc/sw-version is important, it
> should be done and its result cannot be ignored. The logic is already in
> place if cod eis moved into an own handler because switch/reboot is done
> as last step.
>
> >
> >
> > (I probably wouldn't use this for the forseeable future anyway given my
> > update scripts need to be backwards compatible with previous swupdate
> > versions anyway, so I'd rather keep using the "update sw-versions" logic
> > I provide to ensure it doesn't regress more easily, but I think it's a
> > valid pattern even if I'd tend to side with the "generate sw-versions on
> > boot" from an integrator point of view)
>
> Sure, but Markus raises the issue and this can be a new feature that can
> be used or not, and this is easier if it is managed by a separate
> handler instead of core code like in the patches.
>
> Best regards,
> Stefano
>

Best regards,
Marcus Folkesson

[Marcus Folkesson]

Stefano,

On Mon, Apr 22, 2024 at 09:35:47AM +0200, Stefano Babic wrote:

> Hi Dominique, Markus,
>
> On 22.04.24 06:10, Dominique MARTINET wrote:
> > Stefano Babic wrote on Fri, Apr 19, 2024 at 11:57:32AM +0200:
> > > The biggest concern is what is happening if writing the new sw-versions
> > > is failing. The update was successful, but what is happening the next
> > > time if your update is depending on it ?
> >
> > For reference I am doing something similar, and made writing sw-versions
> > mandatory as part of the last step of the update e.g. if that fails then
> > the update isn't applied and the system keeps running the old version.
> >
>
> This is my concern, too. If sw-versions is part of the update, its
> generation must be successful, or the update fails.
>
> Markus' patches introduces this feature as part of the core, and then I
> think it must be coherent. Patches creates a sort of "post command",
> that is executed, but its result is not relevant for the update. The
> command is always executed, independently from the update / SWU.
>
> However, I think that the result is relevant and if it fails, old
> version should be retained. Instead of having some script like Dominique

Thinking about this again :-)

If the result of generate_swversions() is probagated to the caller of
install_images(), the installation procedure will be reversed, right?

IOW:

diff --git a/core/installer.c b/core/installer.c
index c0ea1555..f99fdc1a 100644
--- a/core/installer.c
+++ b/core/installer.c
@@ -470,8 +470,9 @@ int install_images(struct swupdate_cfg *sw)

* Should we generate a list with installed software?

*/
if (strlen(sw->output_swversions)) {
- if (generate_swversions(sw)) {
- WARN("%s cannot be opened", sw->output_swversions);
+ ret |= generate_swversions(sw);
+ if (ret) {

+ ERROR("%s cannot be opened", sw->output_swversions);
}
}

I also think that if swupdate is configured to use --gen-swversion, then the system design
should consider the update as a failure if we did not managed to write the
file.

[Marcus Folkesson]

Any thoughts about this?

Thanks,

Med vänliga hälsningar / Best regards
Marcus Folkesson

[Marcus Folkesson]

After a successful update, --gen-swversions generates a file with
the installed software and their versions.

The file is compatible with CONFIG_SW_VERSIONS_FILE and could replace
that file if the end user want swupdate to maintain that file entirely.

Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
---

Notes:
v2:
- reverse logic for:
if (generate_swversions(sw)) {

ERROR("%s cannot be opened", sw->output_swversions);
}

v3:
- Change ERROR to WARN
- Handle special cases in update_installed_image_version()
- Add gen-swversions config parameter

v4:
- Moved back to ERROR and return failure if swversions could
not be generated

core/installer.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++

core/swupdate.c | 9 ++++++-
include/swupdate.h | 1 +

3 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/core/installer.c b/core/installer.c
index 48c7fe5e..f99fdc1a 100644
--- a/core/installer.c
+++ b/core/installer.c

@@ -409,6 +466,16 @@ int install_images(struct swupdate_cfg *sw)

ret |= run_prepost_scripts(&sw->bootscripts, POSTINSTALL);

+ /*
+ * Should we generate a list with installed software?
+ */
+ if (strlen(sw->output_swversions)) {

+ ret |= generate_swversions(sw);
+ if (ret) {
+ ERROR("%s cannot be opened", sw->output_swversions);

[Marcus Folkesson]

Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
---

[Marcus Folkesson]

Hi all,

Any feedback on this one?

Thanks,
Marcus Folkesson

[Stefano Babic]

Hi Marcus,

On 14.05.24 15:22, Marcus Folkesson wrote:
> Hi all,
>
> Any feedback on this one?
>

Patches look fine for me, I will run coverity before merging.

Regards,
Stefano Babic

> Thanks,
> Marcus Folkesson
>

[Stefano Babic]

Hi Markus,

coverity found an issue that I haven't see at first glance:

If sw_ver_list is NULL, code goes on until LIST_INSERT_HEAD on a NULL
pointer is called.

Best regards,
Stefano Babic

[Marcus Folkesson]

Hi Stefano,

On Wed, May 15, 2024 at 09:19:37AM +0200, Stefano Babic wrote:
> > +static int update_installed_image_version(struct swver *sw_ver_list,
> > + struct img_type *img)
> > +{
> > + struct sw_version *swver;
> > + struct sw_version *swcomp;

if (!sw_ver_list)
return false;

> > +
> > + if (sw_ver_list) {
> > + LIST_FOREACH(swver, sw_ver_list, next) {
> > + /*
> > + * If component is already installed, update the version
> > + */
> > + if (!strncmp(img->id.name, swver->name, sizeof(img->id.name))) {
> > + strncpy(swver->version, img->id.version, sizeof(img->id.version));
> > + return true;
> > + }
> > + }
> > + }
>
> If sw_ver_list is NULL, code goes on until LIST_INSERT_HEAD on a NULL
> pointer is called.

Good catch! May I ask which coverity software you are using?

I will implement the two lines above and send out a v5 in short.

>
> Best regards,

[Marcus Folkesson]

After a successful update, --gen-swversions generates a file with
the installed software and their versions.

The file is compatible with CONFIG_SW_VERSIONS_FILE and could replace
that file if the end user want swupdate to maintain that file entirely.

Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
---

Notes:
v2:
- reverse logic for:
if (generate_swversions(sw)) {
ERROR("%s cannot be opened", sw->output_swversions);
}
v3:
- Change ERROR to WARN
- Handle special cases in update_installed_image_version()
- Add gen-swversions config parameter
v4:
- Moved back to ERROR and return failure if swversions could
not be generated

v5:
- Check if sw_ver_list is NULL in update_installed_image_version

core/installer.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++
core/swupdate.c | 9 +++++-
include/swupdate.h | 1 +
3 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/core/installer.c b/core/installer.c
index 48c7fe5e..cc5ca34b 100644

--- a/core/installer.c
+++ b/core/installer.c
@@ -187,6 +187,24 @@ static int prepare_var_script(struct dict *dict, const char *script)
return 0;
}

+static int generate_swversions(struct swupdate_cfg *cfg)
+{
+ FILE *fp;

+ struct sw_version *swver;

+ struct swver *sw_ver_list = &cfg->installed_sw_list;
+
+ fp = fopen(cfg->output_swversions, "w");
+ if (!fp)
+ return -EACCES;
+

+ LIST_FOREACH(swver, sw_ver_list, next) {

+ fprintf(fp, "%s\t\t%s\n", swver->name, swver->version);
+ }
+ fclose(fp);
+
+ return 0;
+}
+
static int update_bootloader_env(struct swupdate_cfg *cfg, const char *script)
{
int ret = 0;

@@ -279,6 +297,44 @@ int install_single_image(struct img_type *img, bool dry_run)
return ret;
}

+static int update_installed_image_version(struct swver *sw_ver_list,
+ struct img_type *img)
+{
+ struct sw_version *swver;
+ struct sw_version *swcomp;

+
+ if (!sw_ver_list)
+ return false;
+

+ LIST_FOREACH(swver, sw_ver_list, next) {
+ /*
+ * If component is already installed, update the version
+ */
+ if (!strncmp(img->id.name, swver->name, sizeof(img->id.name))) {
+ strncpy(swver->version, img->id.version, sizeof(img->id.version));
+ return true;
+ }
+ }
+

+ if (!strlen(img->id.version))
+ return false;
+
+ /*
+ * No previous version of this component is installed. Create a new entry.
+ */
+ swcomp = (struct sw_version *)calloc(1, sizeof(struct sw_version));
+ if (!swcomp) {
+ ERROR("Could not create new version entry.");
+ return false;
+ }
+
+ strlcpy(swcomp->name, img->id.name, sizeof(swcomp->name));
+ strlcpy(swcomp->version, img->id.version, sizeof(swcomp->version));
+ LIST_INSERT_HEAD(sw_ver_list, swcomp, next);
+

+ return true;
+}
+

/*
* streamfd: file descriptor if it is required to extract
* images from the stream (update from file)

@@ -370,6 +426,8 @@ int install_images(struct swupdate_cfg *sw)

close(img->fdin);

+ update_installed_image_version(&sw->installed_sw_list, img);
+
if (dropimg)
free_image(img);

@@ -409,6 +467,16 @@ int install_images(struct swupdate_cfg *sw)

--
2.44.0

[Marcus Folkesson]

Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
---

[Stefano Babic]

https://scan.coverity.com/projects/sbabic-swupdate?tab=overview

CI is triggering the tool - see .gitlab-ci.yml for detail.

Best regards,
Stefano Babic

[Stefano Babic]

On 15.05.24 09:56, Marcus Folkesson wrote:
> After a successful update, --gen-swversions generates a file with
> the installed software and their versions.
>
> The file is compatible with CONFIG_SW_VERSIONS_FILE and could replace
> that file if the end user want swupdate to maintain that file entirely.
>
> Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
> ---

Applied to -master, thanks !

Best regards,
Stefano Babic

[Stefano Babic]

On 15.05.24 09:56, Marcus Folkesson wrote:

> Signed-off-by: Marcus Folkesson <marcus.f...@gmail.com>
> ---
> doc/source/swupdate.rst | 2 ++
> examples/configuration/swupdate.cfg | 1 +
> 2 files changed, 3 insertions(+)
>

[Paul Sigl]

Hi,

I hope it's okay to reuse this thread for another question/concern regarding this feature.

I noticed that no version file is written for images where "installed-directly" is set. There is a "continue" statement in the install_images method for directly installed images preventing the version entries to be updated. Is this intentional?

Best regards,
Paul Sigl

[Mark Deneen]

I tracked this down as well and then found this message.  Is there any way to use this feature with a streaming handler?

-M

[Mark Deneen]

This patch seems to address the issue.  Stefano, if you are in agreement I can take the steps to get it submitted formally or you can just apply it.  It's up to you.