| Message ID | 20200714090228.4020-2-sde@unmatched.eu |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix install-if-different breakage | expand |
On 14.07.20 11:02, Stijn Devriendt wrote: > From: Stefano Babic <sbabic@denx.de> > > Semantic version are now allowed, but each version is set to be compared > as semantic version. If the string does not match the requirements, > version is artificially set to 0. This breaks compatibility with past > because simple string comparison with any char was possible. > > Reassemble the order of the comparison as: > > - if versions are in format x.y.t.z, then compare them > - if versions are suitable for semantic version, compare them > - else simple use strcmp to return if they match > > Signed-off-by: Stefano Babic <sbabic@denx.de> > [sde@unmatched.eu: dropped trace from find_root_libconfig, > dropped cleanup_version, always clean semvers to avoid memleak, add > debug/trace logs] These 3 lines above are not part of the Signed-off and should be dropped. > Signed-off-by: Stijn Devriendt <sde@unmatched.eu> > --- > core/artifacts_versions.c | 70 ++++++++++++++++----------------------- > core/parser.c | 2 +- > core/swupdate.c | 2 -- > include/util.h | 1 - > parser/parser.c | 3 -- > 5 files changed, 30 insertions(+), 48 deletions(-) > > diff --git a/core/artifacts_versions.c b/core/artifacts_versions.c > index 06c978a..7d1d035 100644 > --- a/core/artifacts_versions.c > +++ b/core/artifacts_versions.c > @@ -68,8 +68,6 @@ static int read_sw_version_file(struct swupdate_cfg *sw) > strlcpy(swcomp->name, name, sizeof(swcomp->name)); > strlcpy(swcomp->version, version, sizeof(swcomp->version)); > > - cleanup_version(swcomp->version); > - > LIST_INSERT_HEAD(&sw->installed_sw_list, swcomp, next); > TRACE("Installed %s: Version %s", > swcomp->name, > @@ -122,8 +120,6 @@ static int versions_settings(void *setting, void *data) > GET_FIELD_STRING(LIBCFG_PARSER, elem, "name", swcomp->name); > GET_FIELD_STRING(LIBCFG_PARSER, elem, "version", swcomp->version); > > - cleanup_version(swcomp->version); > - > LIST_INSERT_HEAD(&sw->installed_sw_list, swcomp, next); > TRACE("Installed %s: Version %s", > swcomp->name, > @@ -173,35 +169,6 @@ static bool is_oldstyle_version(const char* version_string) > return true; > } > > -void cleanup_version(char* str) > -{ > - semver_t version = {}; > - int res = semver_clean(str); > - > - /* This is only expected for overly long version strings. > - * Is SWUPDATE_GENERAL_STRING_SIZE > semver.c:MAX_SIZE + 1??? > - */ > - assert(res == 0); > - if (res == -1) { > - WARN("Version string too long. Using 0.0.0 instead!"); > - str[0] = '\0'; > - return; > - } > - > - if (is_oldstyle_version(str)) > - return; > - > - if (semver_parse(str, &version) == 0) { > - *str = '\0'; > - semver_render(&version, str); > - } else { > - WARN("Unparseable version string. Using 0.0.0 instead!"); > - str[0] = '\0'; > - } > - > - semver_free(&version); > -} > - > /* > * convert a version string into a number > * version string is in the format: > @@ -245,6 +212,7 @@ static __u64 version_to_number(const char *version_string) > * - old-style: major.minor.revision.buildinfo > * - semantic versioning: major.minor.patch[-prerelease][+buildinfo] > * see https://semver.org > + * - if neither works, we fallback to lexicographical comparison > * > * Returns -1, 0 or 1 of left is respectively lower than, equal to or greater than right. > */ > @@ -255,6 +223,9 @@ int compare_versions(const char* left_version, const char* right_version) > __u64 left_u64 = version_to_number(left_version); > __u64 right_u64 = version_to_number(right_version); > > + DEBUG("Comparing old-style versions '%s' <-> '%s'", left_version, right_version); > + TRACE("Parsed: '%llu' <-> '%llu'", left_u64, right_u64); > + > if (left_u64 < right_u64) > return -1; > else if (left_u64 > right_u64) > @@ -268,18 +239,35 @@ int compare_versions(const char* left_version, const char* right_version) > semver_t right_sem = {}; > int comparison; > > - /* There's no error checking here. > - * Oldstyle code also defaults to treating unparseable version as 0. > - * Failed semver_parse also leads to 0.0.0 if properly initialized. > + /* > + * Check if semantic version is possible > */ > - semver_parse(left_version, &left_sem); > - semver_parse(right_version, &right_sem); > - > - comparison = semver_compare(left_sem, right_sem); > + if (!semver_parse(left_version, &left_sem) && !semver_parse(right_version, &right_sem)) { > + DEBUG("Comparing semantic versions '%s' <-> '%s'", left_version, right_version); > + if (loglevel >= TRACELEVEL) > + { > + char left_rendered[SWUPDATE_GENERAL_STRING_SIZE]; > + char right_rendered[SWUPDATE_GENERAL_STRING_SIZE]; > + > + left_rendered[0] = right_rendered[0] = '\0'; > + > + semver_render(&left_sem, left_rendered); > + semver_render(&right_sem, right_rendered); > + TRACE("Parsed: '%s' <-> '%s'", left_rendered, right_rendered); > + } > > + comparison = semver_compare(left_sem, right_sem); > + semver_free(&left_sem); > + semver_free(&right_sem); > + return comparison; > + } > semver_free(&left_sem); > semver_free(&right_sem); > Thanks for clarification, I see, semver_free must always be called even if parse fails. What does happen if just semver_parse(left_version, &left_sem) is failing ? semver_parse(right_version, &right_sem) is not called at all. Do we rely on gcc initialization ? (semver_t right_sem = {}) > - return comparison; > + /* > + * Last attempt: just compare the two strings > + */ > + DEBUG("Comparing lexicographically '%s' <-> '%s'", left_version, right_version); > + return strcmp(left_version, right_version); > } > } > diff --git a/core/parser.c b/core/parser.c > index ba0d2a6..7f281cf 100644 > --- a/core/parser.c > +++ b/core/parser.c > @@ -147,7 +147,7 @@ static int is_image_installed(struct swver *sw_ver_list, > * Check if name and version are identical > */ > if (!strncmp(img->id.name, swver->name, sizeof(img->id.name)) && > - !strncmp(img->id.version, swver->version, sizeof(img->id.version))) { > + !compare_versions(img->id.version, swver->version)) { > TRACE("%s(%s) already installed, skipping...", > img->id.name, > img->id.version); > diff --git a/core/swupdate.c b/core/swupdate.c > index 37906b8..cc7e2d3 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -792,13 +792,11 @@ int main(int argc, char **argv) > swcfg.globals.no_downgrading = 1; > strlcpy(swcfg.globals.minimum_version, optarg, > sizeof(swcfg.globals.minimum_version)); > - cleanup_version(swcfg.globals.minimum_version); > break; > case 'R': > swcfg.globals.no_reinstalling = 1; > strlcpy(swcfg.globals.current_version, optarg, > sizeof(swcfg.globals.current_version)); > - cleanup_version(swcfg.globals.current_version); > break; > case 'M': > swcfg.globals.no_transaction_marker = 1; > diff --git a/include/util.h b/include/util.h > index adf585e..b665179 100644 > --- a/include/util.h > +++ b/include/util.h > @@ -216,7 +216,6 @@ size_t snescape(char *dst, size_t n, const char *src); > void freeargs (char **argv); > int get_hw_revision(struct hw_type *hw); > void get_sw_versions(char *cfgfname, struct swupdate_cfg *sw); > -void cleanup_version(char* str); > int compare_versions(const char* left_version, const char* right_version); > int hwid_match(const char* rev, const char* hwrev); > int check_hw_compatibility(struct swupdate_cfg *cfg); > diff --git a/parser/parser.c b/parser/parser.c > index ffc7fa3..dce5180 100644 > --- a/parser/parser.c > +++ b/parser/parser.c > @@ -273,9 +273,6 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im > > GET_FIELD_STRING(p, elem, "name", image->id.name); > GET_FIELD_STRING(p, elem, "version", image->id.version); > - > - cleanup_version(image->id.version); > - > GET_FIELD_STRING(p, elem, "filename", image->fname); > GET_FIELD_STRING(p, elem, "path", image->path); > GET_FIELD_STRING(p, elem, "volume", image->volname); > Best regards, Stefano Babic
Hello Stijn, Am 14.07.2020 um 11:34 schrieb Stefano Babic: > On 14.07.20 11:02, Stijn Devriendt wrote: >> From: Stefano Babic <sbabic@denx.de> >> >> Semantic version are now allowed, but each version is set to be compared >> as semantic version. If the string does not match the requirements, >> version is artificially set to 0. This breaks compatibility with past >> because simple string comparison with any char was possible. >> >> Reassemble the order of the comparison as: >> >> - if versions are in format x.y.t.z, then compare them >> - if versions are suitable for semantic version, compare them >> - else simple use strcmp to return if they match >> >> Signed-off-by: Stefano Babic <sbabic@denx.de> > > >> [sde@unmatched.eu: dropped trace from find_root_libconfig, >> dropped cleanup_version, always clean semvers to avoid memleak, add >> debug/trace logs] > > These 3 lines above are not part of the Signed-off and should be dropped. > >> Signed-off-by: Stijn Devriendt <sde@unmatched.eu> >> --- >> core/artifacts_versions.c | 70 ++++++++++++++++----------------------- >> core/parser.c | 2 +- >> core/swupdate.c | 2 -- >> include/util.h | 1 - >> parser/parser.c | 3 -- >> 5 files changed, 30 insertions(+), 48 deletions(-) Tested current top of swupdate with this patch applied. Tested on an imx8qxp based board. Updating the firmware with flag "install-if-different" set. Behaves now as expected (and before changes from Stijn) Firmware only updated if version string differs. Tested-by: Heiko Schocher <hs@denx.de> bye, Heiko
On Tuesday, July 14, 2020 at 11:34:25 AM UTC+2, Stefano Babic wrote: > > On 14.07.20 11:02, Stijn Devriendt wrote: > > From: Stefano Babic <sba...@denx.de <javascript:>> > > > > Semantic version are now allowed, but each version is set to be compared > > as semantic version. If the string does not match the requirements, > > version is artificially set to 0. This breaks compatibility with past > > because simple string comparison with any char was possible. > > > > Reassemble the order of the comparison as: > > > > - if versions are in format x.y.t.z, then compare them > > - if versions are suitable for semantic version, compare them > > - else simple use strcmp to return if they match > > > > Signed-off-by: Stefano Babic <sba...@denx.de <javascript:>> > > > > [s...@unmatched.eu <javascript:>: dropped trace from > find_root_libconfig, > > dropped cleanup_version, always clean semvers to avoid memleak, add > > debug/trace logs] > > These 3 lines above are not part of the Signed-off and should be dropped. > OK, let me resend with those removed. I took it from https://web.archive.org/web/20160507011446/http://gerrit.googlecode.com/svn/documentation/2.0/user-signedoffby.html but every project is different, of course. > Signed-off-by: Stijn Devriendt <s...@unmatched.eu <javascript:>> > > --- > > core/artifacts_versions.c | 70 ++++++++++++++++----------------------- > > core/parser.c | 2 +- > > core/swupdate.c | 2 -- > > include/util.h | 1 - > > parser/parser.c | 3 -- > > 5 files changed, 30 insertions(+), 48 deletions(-) > > > > diff --git a/core/artifacts_versions.c b/core/artifacts_versions.c > > index 06c978a..7d1d035 100644 > > --- a/core/artifacts_versions.c > > +++ b/core/artifacts_versions.c > > @@ -68,8 +68,6 @@ static int read_sw_version_file(struct swupdate_cfg > *sw) > > strlcpy(swcomp->name, name, > sizeof(swcomp->name)); > > strlcpy(swcomp->version, version, > sizeof(swcomp->version)); > > > > - cleanup_version(swcomp->version); > > - > > LIST_INSERT_HEAD(&sw->installed_sw_list, > swcomp, next); > > TRACE("Installed %s: Version %s", > > swcomp->name, > > @@ -122,8 +120,6 @@ static int versions_settings(void *setting, void > *data) > > GET_FIELD_STRING(LIBCFG_PARSER, elem, "name", > swcomp->name); > > GET_FIELD_STRING(LIBCFG_PARSER, elem, "version", > swcomp->version); > > > > - cleanup_version(swcomp->version); > > - > > LIST_INSERT_HEAD(&sw->installed_sw_list, swcomp, next); > > TRACE("Installed %s: Version %s", > > swcomp->name, > > @@ -173,35 +169,6 @@ static bool is_oldstyle_version(const char* > version_string) > > return true; > > } > > > > -void cleanup_version(char* str) > > -{ > > - semver_t version = {}; > > - int res = semver_clean(str); > > - > > - /* This is only expected for overly long version strings. > > - * Is SWUPDATE_GENERAL_STRING_SIZE > semver.c:MAX_SIZE + 1??? > > - */ > > - assert(res == 0); > > - if (res == -1) { > > - WARN("Version string too long. Using 0.0.0 instead!"); > > - str[0] = '\0'; > > - return; > > - } > > - > > - if (is_oldstyle_version(str)) > > - return; > > - > > - if (semver_parse(str, &version) == 0) { > > - *str = '\0'; > > - semver_render(&version, str); > > - } else { > > - WARN("Unparseable version string. Using 0.0.0 > instead!"); > > - str[0] = '\0'; > > - } > > - > > - semver_free(&version); > > -} > > - > > /* > > * convert a version string into a number > > * version string is in the format: > > @@ -245,6 +212,7 @@ static __u64 version_to_number(const char > *version_string) > > * - old-style: major.minor.revision.buildinfo > > * - semantic versioning: major.minor.patch[-prerelease][+buildinfo] > > * see https://semver.org > > + * - if neither works, we fallback to lexicographical comparison > > * > > * Returns -1, 0 or 1 of left is respectively lower than, equal to or > greater than right. > > */ > > @@ -255,6 +223,9 @@ int compare_versions(const char* left_version, const > char* right_version) > > __u64 left_u64 = version_to_number(left_version); > > __u64 right_u64 = version_to_number(right_version); > > > > + DEBUG("Comparing old-style versions '%s' <-> '%s'", > left_version, right_version); > > + TRACE("Parsed: '%llu' <-> '%llu'", left_u64, > right_u64); > > + > > if (left_u64 < right_u64) > > return -1; > > else if (left_u64 > right_u64) > > @@ -268,18 +239,35 @@ int compare_versions(const char* left_version, > const char* right_version) > > semver_t right_sem = {}; > > int comparison; > > > > - /* There's no error checking here. > > - * Oldstyle code also defaults to treating unparseable > version as 0. > > - * Failed semver_parse also leads to 0.0.0 if properly > initialized. > > + /* > > + * Check if semantic version is possible > > */ > > - semver_parse(left_version, &left_sem); > > - semver_parse(right_version, &right_sem); > > - > > - comparison = semver_compare(left_sem, right_sem); > > + if (!semver_parse(left_version, &left_sem) && > !semver_parse(right_version, &right_sem)) { > > + DEBUG("Comparing semantic versions '%s' <-> > '%s'", left_version, right_version); > > + if (loglevel >= TRACELEVEL) > > + { > > + char > left_rendered[SWUPDATE_GENERAL_STRING_SIZE]; > > + char > right_rendered[SWUPDATE_GENERAL_STRING_SIZE]; > > + > > + left_rendered[0] = right_rendered[0] = > '\0'; > > + > > + semver_render(&left_sem, > left_rendered); > > + semver_render(&right_sem, > right_rendered); > > + TRACE("Parsed: '%s' <-> '%s'", > left_rendered, right_rendered); > > + } > > > > + comparison = semver_compare(left_sem, > right_sem); > > + semver_free(&left_sem); > > + semver_free(&right_sem); > > + return comparison; > > + } > > semver_free(&left_sem); > > semver_free(&right_sem); > > > > Thanks for clarification, I see, semver_free must always be called even > if parse fails. > > What does happen if just semver_parse(left_version, &left_sem) is > failing ? semver_parse(right_version, &right_sem) is not called at all. > Do we rely on gcc initialization ? (semver_t right_sem = {}) > > Yes, we rely on the zero-initialization by the compiler (should be according to 6.7.8 §21 of the C99 standard). I verified that semver_free is idempotent in this case - iow, it's always OK to call semver_free once the semver_t has been zero-initialized. Regards, Stijn > > > - return comparison; > > + /* > > + * Last attempt: just compare the two strings > > + */ > > + DEBUG("Comparing lexicographically '%s' <-> '%s'", > left_version, right_version); > > + return strcmp(left_version, right_version); > > } > > } > > diff --git a/core/parser.c b/core/parser.c > > index ba0d2a6..7f281cf 100644 > > --- a/core/parser.c > > +++ b/core/parser.c > > @@ -147,7 +147,7 @@ static int is_image_installed(struct swver > *sw_ver_list, > > * Check if name and version are identical > > */ > > if (!strncmp(img->id.name, swver->name, sizeof(img-> > id.name)) && > > - !strncmp(img->id.version, swver->version, > sizeof(img->id.version))) { > > + !compare_versions(img->id.version, swver->version)) > { > > TRACE("%s(%s) already installed, skipping...", > > img->id.name, > > img->id.version); > > diff --git a/core/swupdate.c b/core/swupdate.c > > index 37906b8..cc7e2d3 100644 > > --- a/core/swupdate.c > > +++ b/core/swupdate.c > > @@ -792,13 +792,11 @@ int main(int argc, char **argv) > > swcfg.globals.no_downgrading = 1; > > strlcpy(swcfg.globals.minimum_version, optarg, > > sizeof(swcfg.globals.minimum_version)); > > - cleanup_version(swcfg.globals.minimum_version); > > break; > > case 'R': > > swcfg.globals.no_reinstalling = 1; > > strlcpy(swcfg.globals.current_version, optarg, > > sizeof(swcfg.globals.current_version)); > > - cleanup_version(swcfg.globals.current_version); > > break; > > case 'M': > > swcfg.globals.no_transaction_marker = 1; > > diff --git a/include/util.h b/include/util.h > > index adf585e..b665179 100644 > > --- a/include/util.h > > +++ b/include/util.h > > @@ -216,7 +216,6 @@ size_t snescape(char *dst, size_t n, const char > *src); > > void freeargs (char **argv); > > int get_hw_revision(struct hw_type *hw); > > void get_sw_versions(char *cfgfname, struct swupdate_cfg *sw); > > -void cleanup_version(char* str); > > int compare_versions(const char* left_version, const char* > right_version); > > int hwid_match(const char* rev, const char* hwrev); > > int check_hw_compatibility(struct swupdate_cfg *cfg); > > diff --git a/parser/parser.c b/parser/parser.c > > index ffc7fa3..dce5180 100644 > > --- a/parser/parser.c > > +++ b/parser/parser.c > > @@ -273,9 +273,6 @@ static int parse_common_attributes(parsertype p, > void *elem, struct img_type *im > > > > GET_FIELD_STRING(p, elem, "name", image->id.name); > > GET_FIELD_STRING(p, elem, "version", image->id.version); > > - > > - cleanup_version(image->id.version); > > - > > GET_FIELD_STRING(p, elem, "filename", image->fname); > > GET_FIELD_STRING(p, elem, "path", image->path); > > GET_FIELD_STRING(p, elem, "volume", image->volname); > > > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de > <javascript:> > ===================================================================== >
On 14.07.20 11:54, highguy@gmail.com wrote: > On Tuesday, July 14, 2020 at 11:34:25 AM UTC+2, Stefano Babic wrote: > > On 14.07.20 11:02, Stijn Devriendt wrote: > > From: Stefano Babic <sba...@denx.de <javascript:>> > > > > Semantic version are now allowed, but each version is set to be > compared > > as semantic version. If the string does not match the requirements, > > version is artificially set to 0. This breaks compatibility with past > > because simple string comparison with any char was possible. > > > > Reassemble the order of the comparison as: > > > > - if versions are in format x.y.t.z, then compare them > > - if versions are suitable for semantic version, compare them > > - else simple use strcmp to return if they match > > > > Signed-off-by: Stefano Babic <sba...@denx.de <javascript:>> > > > > [s...@unmatched.eu <javascript:>: dropped trace from > find_root_libconfig, > > dropped cleanup_version, always clean semvers to avoid memleak, add > > debug/trace logs] > > These 3 lines above are not part of the Signed-off and should be > dropped. > > > OK, let me resend with those removed. I took it from > https://web.archive.org/web/20160507011446/http://gerrit.googlecode.com/svn/documentation/2.0/user-signedoffby.html > but every project is different, of course. > > > Signed-off-by: Stijn Devriendt <s...@unmatched.eu <javascript:>> > > --- > > core/artifacts_versions.c | 70 > ++++++++++++++++----------------------- > > core/parser.c | 2 +- > > core/swupdate.c | 2 -- > > include/util.h | 1 - > > parser/parser.c | 3 -- > > 5 files changed, 30 insertions(+), 48 deletions(-) > > > > diff --git a/core/artifacts_versions.c b/core/artifacts_versions.c > > index 06c978a..7d1d035 100644 > > --- a/core/artifacts_versions.c > > +++ b/core/artifacts_versions.c > > @@ -68,8 +68,6 @@ static int read_sw_version_file(struct > swupdate_cfg *sw) > > strlcpy(swcomp->name, name, > sizeof(swcomp->name)); > > strlcpy(swcomp->version, version, > sizeof(swcomp->version)); > > > > - cleanup_version(swcomp->version); > > - > > LIST_INSERT_HEAD(&sw->installed_sw_list, > swcomp, next); > > TRACE("Installed %s: Version %s", > > swcomp->name, > > @@ -122,8 +120,6 @@ static int versions_settings(void *setting, > void *data) > > GET_FIELD_STRING(LIBCFG_PARSER, elem, "name", > swcomp->name); > > GET_FIELD_STRING(LIBCFG_PARSER, elem, "version", > swcomp->version); > > > > - cleanup_version(swcomp->version); > > - > > LIST_INSERT_HEAD(&sw->installed_sw_list, swcomp, > next); > > TRACE("Installed %s: Version %s", > > swcomp->name, > > @@ -173,35 +169,6 @@ static bool is_oldstyle_version(const char* > version_string) > > return true; > > } > > > > -void cleanup_version(char* str) > > -{ > > - semver_t version = {}; > > - int res = semver_clean(str); > > - > > - /* This is only expected for overly long version strings. > > - * Is SWUPDATE_GENERAL_STRING_SIZE > semver.c:MAX_SIZE + > 1??? > > - */ > > - assert(res == 0); > > - if (res == -1) { > > - WARN("Version string too long. Using 0.0.0 > instead!"); > > - str[0] = '\0'; > > - return; > > - } > > - > > - if (is_oldstyle_version(str)) > > - return; > > - > > - if (semver_parse(str, &version) == 0) { > > - *str = '\0'; > > - semver_render(&version, str); > > - } else { > > - WARN("Unparseable version string. Using 0.0.0 > instead!"); > > - str[0] = '\0'; > > - } > > - > > - semver_free(&version); > > -} > > - > > /* > > * convert a version string into a number > > * version string is in the format: > > @@ -245,6 +212,7 @@ static __u64 version_to_number(const char > *version_string) > > * - old-style: major.minor.revision.buildinfo > > * - semantic versioning: major.minor.patch[-prerelease][+buildinfo] > > * see https://semver.org > > + * - if neither works, we fallback to lexicographical comparison > > * > > * Returns -1, 0 or 1 of left is respectively lower than, equal > to or greater than right. > > */ > > @@ -255,6 +223,9 @@ int compare_versions(const char* left_version, > const char* right_version) > > __u64 left_u64 = version_to_number(left_version); > > __u64 right_u64 = version_to_number(right_version); > > > > + DEBUG("Comparing old-style versions '%s' <-> > '%s'", left_version, right_version); > > + TRACE("Parsed: '%llu' <-> '%llu'", left_u64, > right_u64); > > + > > if (left_u64 < right_u64) > > return -1; > > else if (left_u64 > right_u64) > > @@ -268,18 +239,35 @@ int compare_versions(const char* > left_version, const char* right_version) > > semver_t right_sem = {}; > > int comparison; > > > > - /* There's no error checking here. > > - * Oldstyle code also defaults to treating > unparseable version as 0. > > - * Failed semver_parse also leads to 0.0.0 if > properly initialized. > > + /* > > + * Check if semantic version is possible > > */ > > - semver_parse(left_version, &left_sem); > > - semver_parse(right_version, &right_sem); > > - > > - comparison = semver_compare(left_sem, right_sem); > > + if (!semver_parse(left_version, &left_sem) && > !semver_parse(right_version, &right_sem)) { > > + DEBUG("Comparing semantic versions '%s' > <-> '%s'", left_version, right_version); > > + if (loglevel >= TRACELEVEL) > > + { > > + char > left_rendered[SWUPDATE_GENERAL_STRING_SIZE]; > > + char > right_rendered[SWUPDATE_GENERAL_STRING_SIZE]; > > + > > + left_rendered[0] = > right_rendered[0] = '\0'; > > + > > + semver_render(&left_sem, > left_rendered); > > + semver_render(&right_sem, > right_rendered); > > + TRACE("Parsed: '%s' <-> '%s'", > left_rendered, right_rendered); > > + } > > > > + comparison = semver_compare(left_sem, > right_sem); > > + semver_free(&left_sem); > > + semver_free(&right_sem); > > + return comparison; > > + } > > semver_free(&left_sem); > > semver_free(&right_sem); > > > > Thanks for clarification, I see, semver_free must always be called even > if parse fails. > > What does happen if just semver_parse(left_version, &left_sem) is > failing ? semver_parse(right_version, &right_sem) is not called at all. > Do we rely on gcc initialization ? (semver_t right_sem = {}) > > > Yes, we rely on the zero-initialization by the compiler (should be according > to 6.7.8 §21 of the C99 standard). I verified that semver_free is idempotent > in this case Perfect ! >- iow, it's always OK to call semver_free once the semver_t > has been > zero-initialized. > Ok, I merge the patch. Thanks, Stefano > Regards, > Stijn > > > > - return comparison; > > + /* > > + * Last attempt: just compare the two strings > > + */ > > + DEBUG("Comparing lexicographically '%s' <-> > '%s'", left_version, right_version); > > + return strcmp(left_version, right_version); > > } > > } > > diff --git a/core/parser.c b/core/parser.c > > index ba0d2a6..7f281cf 100644 > > --- a/core/parser.c > > +++ b/core/parser.c > > @@ -147,7 +147,7 @@ static int is_image_installed(struct swver > *sw_ver_list, > > * Check if name and version are identical > > */ > > if (!strncmp(img->id.name <http://id.name>, > swver->name, sizeof(img->id.name <http://id.name>)) && > > - !strncmp(img->id.version, swver->version, > sizeof(img->id.version))) { > > + !compare_versions(img->id.version, > swver->version)) { > > TRACE("%s(%s) already installed, > skipping...", > > img->id.name <http://id.name>, > > img->id.version); > > diff --git a/core/swupdate.c b/core/swupdate.c > > index 37906b8..cc7e2d3 100644 > > --- a/core/swupdate.c > > +++ b/core/swupdate.c > > @@ -792,13 +792,11 @@ int main(int argc, char **argv) > > swcfg.globals.no_downgrading = 1; > > strlcpy(swcfg.globals.minimum_version, > optarg, > > > sizeof(swcfg.globals.minimum_version)); > > > > - cleanup_version(swcfg.globals.minimum_version); > > > break; > > case 'R': > > swcfg.globals.no_reinstalling = 1; > > strlcpy(swcfg.globals.current_version, > optarg, > > > sizeof(swcfg.globals.current_version)); > > > > - cleanup_version(swcfg.globals.current_version); > > > break; > > case 'M': > > swcfg.globals.no_transaction_marker = 1; > > diff --git a/include/util.h b/include/util.h > > index adf585e..b665179 100644 > > --- a/include/util.h > > +++ b/include/util.h > > @@ -216,7 +216,6 @@ size_t snescape(char *dst, size_t n, const > char *src); > > void freeargs (char **argv); > > int get_hw_revision(struct hw_type *hw); > > void get_sw_versions(char *cfgfname, struct swupdate_cfg *sw); > > -void cleanup_version(char* str); > > int compare_versions(const char* left_version, const char* > right_version); > > int hwid_match(const char* rev, const char* hwrev); > > int check_hw_compatibility(struct swupdate_cfg *cfg); > > diff --git a/parser/parser.c b/parser/parser.c > > index ffc7fa3..dce5180 100644 > > --- a/parser/parser.c > > +++ b/parser/parser.c > > @@ -273,9 +273,6 @@ static int parse_common_attributes(parsertype > p, void *elem, struct img_type *im > > > > GET_FIELD_STRING(p, elem, "name", image->id.name > <http://id.name>); > > GET_FIELD_STRING(p, elem, "version", image->id.version); > > - > > - cleanup_version(image->id.version); > > - > > GET_FIELD_STRING(p, elem, "filename", image->fname); > > GET_FIELD_STRING(p, elem, "path", image->path); > > GET_FIELD_STRING(p, elem, "volume", image->volname); > > > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: > sba...@denx.de <javascript:> > ===================================================================== > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/8c55fddf-dcb6-458e-8a0e-f42da1610143o%40googlegroups.com > <https://groups.google.com/d/msgid/swupdate/8c55fddf-dcb6-458e-8a0e-f42da1610143o%40googlegroups.com?utm_medium=email&utm_source=footer>.
diff --git a/core/artifacts_versions.c b/core/artifacts_versions.c index 06c978a..7d1d035 100644 --- a/core/artifacts_versions.c +++ b/core/artifacts_versions.c @@ -68,8 +68,6 @@ static int read_sw_version_file(struct swupdate_cfg *sw) strlcpy(swcomp->name, name, sizeof(swcomp->name)); strlcpy(swcomp->version, version, sizeof(swcomp->version)); - cleanup_version(swcomp->version); - LIST_INSERT_HEAD(&sw->installed_sw_list, swcomp, next); TRACE("Installed %s: Version %s", swcomp->name, @@ -122,8 +120,6 @@ static int versions_settings(void *setting, void *data) GET_FIELD_STRING(LIBCFG_PARSER, elem, "name", swcomp->name); GET_FIELD_STRING(LIBCFG_PARSER, elem, "version", swcomp->version); - cleanup_version(swcomp->version); - LIST_INSERT_HEAD(&sw->installed_sw_list, swcomp, next); TRACE("Installed %s: Version %s", swcomp->name, @@ -173,35 +169,6 @@ static bool is_oldstyle_version(const char* version_string) return true; } -void cleanup_version(char* str) -{ - semver_t version = {}; - int res = semver_clean(str); - - /* This is only expected for overly long version strings. - * Is SWUPDATE_GENERAL_STRING_SIZE > semver.c:MAX_SIZE + 1??? - */ - assert(res == 0); - if (res == -1) { - WARN("Version string too long. Using 0.0.0 instead!"); - str[0] = '\0'; - return; - } - - if (is_oldstyle_version(str)) - return; - - if (semver_parse(str, &version) == 0) { - *str = '\0'; - semver_render(&version, str); - } else { - WARN("Unparseable version string. Using 0.0.0 instead!"); - str[0] = '\0'; - } - - semver_free(&version); -} - /* * convert a version string into a number * version string is in the format: @@ -245,6 +212,7 @@ static __u64 version_to_number(const char *version_string) * - old-style: major.minor.revision.buildinfo * - semantic versioning: major.minor.patch[-prerelease][+buildinfo] * see https://semver.org + * - if neither works, we fallback to lexicographical comparison * * Returns -1, 0 or 1 of left is respectively lower than, equal to or greater than right. */ @@ -255,6 +223,9 @@ int compare_versions(const char* left_version, const char* right_version) __u64 left_u64 = version_to_number(left_version); __u64 right_u64 = version_to_number(right_version); + DEBUG("Comparing old-style versions '%s' <-> '%s'", left_version, right_version); + TRACE("Parsed: '%llu' <-> '%llu'", left_u64, right_u64); + if (left_u64 < right_u64) return -1; else if (left_u64 > right_u64) @@ -268,18 +239,35 @@ int compare_versions(const char* left_version, const char* right_version) semver_t right_sem = {}; int comparison; - /* There's no error checking here. - * Oldstyle code also defaults to treating unparseable version as 0. - * Failed semver_parse also leads to 0.0.0 if properly initialized. + /* + * Check if semantic version is possible */ - semver_parse(left_version, &left_sem); - semver_parse(right_version, &right_sem); - - comparison = semver_compare(left_sem, right_sem); + if (!semver_parse(left_version, &left_sem) && !semver_parse(right_version, &right_sem)) { + DEBUG("Comparing semantic versions '%s' <-> '%s'", left_version, right_version); + if (loglevel >= TRACELEVEL) + { + char left_rendered[SWUPDATE_GENERAL_STRING_SIZE]; + char right_rendered[SWUPDATE_GENERAL_STRING_SIZE]; + + left_rendered[0] = right_rendered[0] = '\0'; + + semver_render(&left_sem, left_rendered); + semver_render(&right_sem, right_rendered); + TRACE("Parsed: '%s' <-> '%s'", left_rendered, right_rendered); + } + comparison = semver_compare(left_sem, right_sem); + semver_free(&left_sem); + semver_free(&right_sem); + return comparison; + } semver_free(&left_sem); semver_free(&right_sem); - return comparison; + /* + * Last attempt: just compare the two strings + */ + DEBUG("Comparing lexicographically '%s' <-> '%s'", left_version, right_version); + return strcmp(left_version, right_version); } } diff --git a/core/parser.c b/core/parser.c index ba0d2a6..7f281cf 100644 --- a/core/parser.c +++ b/core/parser.c @@ -147,7 +147,7 @@ static int is_image_installed(struct swver *sw_ver_list, * Check if name and version are identical */ if (!strncmp(img->id.name, swver->name, sizeof(img->id.name)) && - !strncmp(img->id.version, swver->version, sizeof(img->id.version))) { + !compare_versions(img->id.version, swver->version)) { TRACE("%s(%s) already installed, skipping...", img->id.name, img->id.version); diff --git a/core/swupdate.c b/core/swupdate.c index 37906b8..cc7e2d3 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -792,13 +792,11 @@ int main(int argc, char **argv) swcfg.globals.no_downgrading = 1; strlcpy(swcfg.globals.minimum_version, optarg, sizeof(swcfg.globals.minimum_version)); - cleanup_version(swcfg.globals.minimum_version); break; case 'R': swcfg.globals.no_reinstalling = 1; strlcpy(swcfg.globals.current_version, optarg, sizeof(swcfg.globals.current_version)); - cleanup_version(swcfg.globals.current_version); break; case 'M': swcfg.globals.no_transaction_marker = 1; diff --git a/include/util.h b/include/util.h index adf585e..b665179 100644 --- a/include/util.h +++ b/include/util.h @@ -216,7 +216,6 @@ size_t snescape(char *dst, size_t n, const char *src); void freeargs (char **argv); int get_hw_revision(struct hw_type *hw); void get_sw_versions(char *cfgfname, struct swupdate_cfg *sw); -void cleanup_version(char* str); int compare_versions(const char* left_version, const char* right_version); int hwid_match(const char* rev, const char* hwrev); int check_hw_compatibility(struct swupdate_cfg *cfg); diff --git a/parser/parser.c b/parser/parser.c index ffc7fa3..dce5180 100644 --- a/parser/parser.c +++ b/parser/parser.c @@ -273,9 +273,6 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im GET_FIELD_STRING(p, elem, "name", image->id.name); GET_FIELD_STRING(p, elem, "version", image->id.version); - - cleanup_version(image->id.version); - GET_FIELD_STRING(p, elem, "filename", image->fname); GET_FIELD_STRING(p, elem, "path", image->path); GET_FIELD_STRING(p, elem, "volume", image->volname);