| Message ID | 20240419084821.610411-1-marcus.folkesson@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] Introduce --gen-swversions parameter | expand |
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 ? > Signed-off-by: Marcus Folkesson <marcus.folkesson@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 > --- 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; 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. > + } > + } > + > + /* > + * 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); An ERROR in SWUpdate stops the update, this should scaled down at least to WARN. > + } > + } > + > 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]; 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
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. > > > Signed-off-by: Marcus Folkesson <marcus.folkesson@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 > > --- 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; > > 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. 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. > > + > > + 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); > > An ERROR in SWUpdate stops the update, this should scaled down at least > to WARN. 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 :-) > > {"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]; > > 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. Got it, thank you. > > Best regards, > Stefano Babic
Hi Markus, On 19.04.24 21:53, Marcus Folkesson wrote: > 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. > >> >> > Signed-off-by: Marcus Folkesson <marcus.folkesson@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 >>> --- 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; >> >> 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. > > Hum hum, not sure if I get you. > Does not you talk about... 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 >>> + >>> + 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); >> >> An ERROR in SWUpdate stops the update, this should scaled down at least >> to WARN. > > 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 :-) > 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 >>> {"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]; >> >> 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. > > Got it, thank you. > >> >> Best regards, >> Stefano Babic >
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)
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
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
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. > Best regards, > Stefano > Best regards, Marcus Folkesson
Any thoughts about this? Thanks, Med vänliga hälsningar / Best regards Marcus Folkesson Den tis 23 apr. 2024 kl 22:25 skrev Marcus Folkesson < marcus.folkesson@gmail.com>: > 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. > > > > Best regards, > > Stefano > > > > Best regards, > Marcus Folkesson >
diff --git a/core/installer.c b/core/installer.c index 48c7fe5e..bbcc0497 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];
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.folkesson@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(-)