| Message ID | 20240507080243.725871-1-marcus.folkesson@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] Introduce --gen-swversions parameter | expand |
Hi all, Any feedback on this one? Thanks, Marcus Folkesson
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 >
Hi Markus, coverity found an issue that I haven't see at first glance: On 07.05.24 10:02, 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.folkesson@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 > @@ -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 sw_ver_list is NULL, code goes on until LIST_INSERT_HEAD on a NULL pointer is called. Best regards, Stefano Babic > + > + 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,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); > + } > + } > + > 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];
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, > Stefano Babic Thanks, Marcus Folkesson
On 15.05.24 09:33, Marcus Folkesson wrote: > 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? > https://scan.coverity.com/projects/sbabic-swupdate?tab=overview CI is triggering the tool - see .gitlab-ci.yml for detail. Best regards, Stefano Babic > I will implement the two lines above and send out a v5 in short. > >> >> Best regards, >> Stefano Babic > > > Thanks, > Marcus Folkesson
diff --git a/core/installer.c b/core/installer.c index 48c7fe5e..f99fdc1a 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,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); + } + } + 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];
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); } 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(-)