[libvirt PATCH v2] ch: Fix cloud-hypervisor version processing
Refactor the version processing logic in ch driver to support versions from non-release cloud-hypervisor binaries. This version also supports versions with branch prefixes in them. Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/ch/ch_conf.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index a8565d9537..a0849bfbd6 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -172,6 +172,33 @@ virCHDriverConfigDispose(void *obj) #define MIN_VERSION ((15 * 1000000) + (0 * 1000) + (0)) +/** + * chPreProcessVersionString - crop branch, commit info and return just the version + */ +static char * +chPreProcessVersionString(char *version) +{ + g_autofree char *new_version = version; + char *tmp_version; + + if ((tmp_version = strrchr(version, '/')) != NULL) { + new_version = tmp_version + 1; + } + + if (new_version[0] == 'v') { + new_version = new_version + 1; + } + // Duplicate the string in both cases. This would allow the calling method + // free the returned string in a consistent manner. + if ((tmp_version = strchr(new_version, '-')) != NULL) { + new_version = g_strndup(new_version, tmp_version - new_version); + } else{ + new_version = g_strdup(new_version); + } + + return g_steal_pointer(&new_version); + +} int chExtractVersion(virCHDriver *driver) { @@ -193,19 +220,25 @@ chExtractVersion(virCHDriver *driver) tmp = help; - /* expected format: cloud-hypervisor v<major>.<minor>.<micro> */ - if ((tmp = STRSKIP(tmp, "cloud-hypervisor v")) == NULL) { + /* Below are some example version formats and expected outputs: + * cloud-hypervisor v32.0.0 (expected: 32.0.0) + * cloud-hypervisor v33.0-104-ge0e3779e-dirty (expected: 33.0) + * cloud-hypervisor testing/v32.0.131-1-ga5d6db5c-dirty (expected: 32.0.131) + */ + if ((tmp = STRSKIP(tmp, "cloud-hypervisor ")) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unexpected output of cloud-hypervisor binary")); return -1; } + tmp = chPreProcessVersionString(tmp); + VIR_DEBUG("Cloud-Hypervisor version detected: %s", tmp); + if (virStringParseVersion(&version, tmp, true) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse cloud-hypervisor version: %1$s"), tmp); return -1; } - if (version < MIN_VERSION) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cloud-Hypervisor version is too old (v15.0 is the minimum supported version)")); -- 2.41.0
On Tue, Sep 05, 2023 at 12:44:25PM -0500, Praveen K Paladugu wrote:
Refactor the version processing logic in ch driver to support versions from non-release cloud-hypervisor binaries. This version also supports versions with branch prefixes in them.
Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/ch/ch_conf.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index a8565d9537..a0849bfbd6 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -172,6 +172,33 @@ virCHDriverConfigDispose(void *obj)
#define MIN_VERSION ((15 * 1000000) + (0 * 1000) + (0))
+/** + * chPreProcessVersionString - crop branch, commit info and return just the version + */ +static char * +chPreProcessVersionString(char *version) +{ + g_autofree char *new_version = version; + char *tmp_version; + + if ((tmp_version = strrchr(version, '/')) != NULL) { + new_version = tmp_version + 1; + } + + if (new_version[0] == 'v') { + new_version = new_version + 1; + }
This is unsafe - 'new_version' is marked g_autofree, and this pointer manipulation will cause it to try to free a location /after/ the start of the allocation. Except you have no error paths, so the g_autofree will never trigger. Remove the g_autofree + g_steal_pointer as they're unsafe if the former ever ran.
+ // Duplicate the string in both cases. This would allow the calling method + // free the returned string in a consistent manner. + if ((tmp_version = strchr(new_version, '-')) != NULL) { + new_version = g_strndup(new_version, tmp_version - new_version); + } else{ + new_version = g_strdup(new_version); + } + + return g_steal_pointer(&new_version); + +} int chExtractVersion(virCHDriver *driver) { @@ -193,19 +220,25 @@ chExtractVersion(virCHDriver *driver)
tmp = help;
- /* expected format: cloud-hypervisor v<major>.<minor>.<micro> */ - if ((tmp = STRSKIP(tmp, "cloud-hypervisor v")) == NULL) { + /* Below are some example version formats and expected outputs: + * cloud-hypervisor v32.0.0 (expected: 32.0.0) + * cloud-hypervisor v33.0-104-ge0e3779e-dirty (expected: 33.0) + * cloud-hypervisor testing/v32.0.131-1-ga5d6db5c-dirty (expected: 32.0.131) + */ + if ((tmp = STRSKIP(tmp, "cloud-hypervisor ")) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unexpected output of cloud-hypervisor binary")); return -1; }
+ tmp = chPreProcessVersionString(tmp); + VIR_DEBUG("Cloud-Hypervisor version detected: %s", tmp); + if (virStringParseVersion(&version, tmp, true) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse cloud-hypervisor version: %1$s"), tmp); return -1; } - if (version < MIN_VERSION) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cloud-Hypervisor version is too old (v15.0 is the minimum supported version)")); -- 2.41.0
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Wed, Sep 06, 2023 at 09:51:11AM +0100, Daniel P. Berrang?? wrote:
On Tue, Sep 05, 2023 at 12:44:25PM -0500, Praveen K Paladugu wrote:
Refactor the version processing logic in ch driver to support versions from non-release cloud-hypervisor binaries. This version also supports versions with branch prefixes in them.
Signed-off-by: Praveen K Paladugu <prapal@linux.microsoft.com> --- src/ch/ch_conf.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index a8565d9537..a0849bfbd6 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -172,6 +172,33 @@ virCHDriverConfigDispose(void *obj)
#define MIN_VERSION ((15 * 1000000) + (0 * 1000) + (0))
+/** + * chPreProcessVersionString - crop branch, commit info and return just the version + */ +static char * +chPreProcessVersionString(char *version) +{ + g_autofree char *new_version = version; + char *tmp_version; + + if ((tmp_version = strrchr(version, '/')) != NULL) { + new_version = tmp_version + 1; + } + + if (new_version[0] == 'v') { + new_version = new_version + 1; + }
This is unsafe - 'new_version' is marked g_autofree, and this pointer manipulation will cause it to try to free a location /after/ the start of the allocation. Except you have no error paths, so the g_autofree will never trigger.
Remove the g_autofree + g_steal_pointer as they're unsafe if the former ever ran.
Thanks Daniel. I fixed this in the newer version. I applied g_autofree only to the returned version. Returned version is not manipulated in the calling method, so it can be freed by g_autofree.
+ // Duplicate the string in both cases. This would allow the calling method + // free the returned string in a consistent manner. + if ((tmp_version = strchr(new_version, '-')) != NULL) { + new_version = g_strndup(new_version, tmp_version - new_version); + } else{ + new_version = g_strdup(new_version); + } + + return g_steal_pointer(&new_version); + +} int chExtractVersion(virCHDriver *driver) { @@ -193,19 +220,25 @@ chExtractVersion(virCHDriver *driver)
tmp = help;
- /* expected format: cloud-hypervisor v<major>.<minor>.<micro> */ - if ((tmp = STRSKIP(tmp, "cloud-hypervisor v")) == NULL) { + /* Below are some example version formats and expected outputs: + * cloud-hypervisor v32.0.0 (expected: 32.0.0) + * cloud-hypervisor v33.0-104-ge0e3779e-dirty (expected: 33.0) + * cloud-hypervisor testing/v32.0.131-1-ga5d6db5c-dirty (expected: 32.0.131) + */ + if ((tmp = STRSKIP(tmp, "cloud-hypervisor ")) == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unexpected output of cloud-hypervisor binary")); return -1; }
+ tmp = chPreProcessVersionString(tmp); + VIR_DEBUG("Cloud-Hypervisor version detected: %s", tmp); + if (virStringParseVersion(&version, tmp, true) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse cloud-hypervisor version: %1$s"), tmp); return -1; } - if (version < MIN_VERSION) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cloud-Hypervisor version is too old (v15.0 is the minimum supported version)")); -- 2.41.0
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
Praveen K Paladugu -
Praveen Paladugu