[libvirt PATCH v3] 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 | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index a8565d9537..5573119b23 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -172,6 +172,37 @@ virCHDriverConfigDispose(void *obj) #define MIN_VERSION ((15 * 1000000) + (0 * 1000) + (0)) +/** + * chPreProcessVersionString: + * + * Returns: a pointer to numerical version without branch/commit info + */ +static char * +chPreProcessVersionString(char *version) +{ + char *tmp_version = version; + g_autofree char *ret_version = NULL; + char *sub_string; + + if ((sub_string = strrchr(version, '/')) != NULL) { + tmp_version = sub_string + 1; + } + + if (tmp_version[0] == 'v') { + tmp_version = tmp_version + 1; + } + + // Duplicate the string in both cases. This would allow the calling method + // free the returned string in a consistent manner. + if ((sub_string = strchr(tmp_version, '-')) != NULL) { + ret_version = g_strndup(tmp_version, sub_string - tmp_version); + } else{ + ret_version = g_strdup(tmp_version); + } + + return g_steal_pointer(&ret_version); + +} int chExtractVersion(virCHDriver *driver) { @@ -193,13 +224,20 @@ 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); -- 2.41.0
On Wed, Sep 06, 2023 at 10:50:30AM -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 | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index a8565d9537..5573119b23 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -172,6 +172,37 @@ virCHDriverConfigDispose(void *obj)
#define MIN_VERSION ((15 * 1000000) + (0 * 1000) + (0))
+/** + * chPreProcessVersionString: + * + * Returns: a pointer to numerical version without branch/commit info + */ +static char * +chPreProcessVersionString(char *version) +{ + char *tmp_version = version; + g_autofree char *ret_version = NULL; + char *sub_string; + + if ((sub_string = strrchr(version, '/')) != NULL) { + tmp_version = sub_string + 1; + } + + if (tmp_version[0] == 'v') { + tmp_version = tmp_version + 1; + } + + // Duplicate the string in both cases. This would allow the calling method + // free the returned string in a consistent manner. + if ((sub_string = strchr(tmp_version, '-')) != NULL) { + ret_version = g_strndup(tmp_version, sub_string - tmp_version); + } else{ + ret_version = g_strdup(tmp_version); + } + + return g_steal_pointer(&ret_version); + +}
What would be wrong with the following? static char * chPreProcessVersionString(const char *version) { const char *tmp = strrchr(version, '/'); if (tmp) version = tmp + 1; if (version[0] == 'v') version++; tmp = strchr(tmp_version, '-'); if (tmp) return g_strndup(version, tmp - version); else return g_strdup(version; } isn't that more readable?
int chExtractVersion(virCHDriver *driver) { @@ -193,13 +224,20 @@ 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); +
Also tmp is not free'd in this function which introduces a leak. But since @help is not used for anything else we could simplify the processing of the version by not duplicating anything: static char * chPreProcessVersionString(char *version) { char *tmp = strrchr(version, '/'); if (tmp) version = tmp + 1; if (version[0] == 'v') version++; tmp = strchr(version, '-'); if (tmp) *tmp = '\0'; return version; } and keep this part just as you changed it.
if (virStringParseVersion(&version, tmp, true) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse cloud-hypervisor version: %1$s"), tmp); -- 2.41.0
On Thu, Sep 07, 2023 at 09:34:59AM +0200, Martin Kletzander wrote:
On Wed, Sep 06, 2023 at 10:50:30AM -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 | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index a8565d9537..5573119b23 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -172,6 +172,37 @@ virCHDriverConfigDispose(void *obj)
#define MIN_VERSION ((15 * 1000000) + (0 * 1000) + (0))
+/** + * chPreProcessVersionString: + * + * Returns: a pointer to numerical version without branch/commit info + */ +static char * +chPreProcessVersionString(char *version) +{ + char *tmp_version = version; + g_autofree char *ret_version = NULL; + char *sub_string; + + if ((sub_string = strrchr(version, '/')) != NULL) { + tmp_version = sub_string + 1; + } + + if (tmp_version[0] == 'v') { + tmp_version = tmp_version + 1; + } + + // Duplicate the string in both cases. This would allow the calling method + // free the returned string in a consistent manner. + if ((sub_string = strchr(tmp_version, '-')) != NULL) { + ret_version = g_strndup(tmp_version, sub_string - tmp_version); + } else{ + ret_version = g_strdup(tmp_version); + } + + return g_steal_pointer(&ret_version); + +}
What would be wrong with the following?
static char * chPreProcessVersionString(const char *version) { const char *tmp = strrchr(version, '/');
if (tmp) version = tmp + 1;
if (version[0] == 'v') version++;
tmp = strchr(tmp_version, '-'); if (tmp) return g_strndup(version, tmp - version); else return g_strdup(version; }
isn't that more readable?
int chExtractVersion(virCHDriver *driver) { @@ -193,13 +224,20 @@ 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); +
Also tmp is not free'd in this function which introduces a leak.
But since @help is not used for anything else we could simplify the processing of the version by not duplicating anything:
static char * chPreProcessVersionString(char *version) { char *tmp = strrchr(version, '/');
if (tmp) version = tmp + 1;
if (version[0] == 'v') version++;
tmp = strchr(version, '-'); if (tmp) *tmp = '\0';
return version; }
and keep this part just as you changed it.
Thanks. I like this version. I will adopt this in my next revision.
if (virStringParseVersion(&version, tmp, true) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse cloud-hypervisor version: %1$s"), tmp); -- 2.41.0
participants (3)
-
Martin Kletzander -
Praveen K Paladugu -
Praveen Paladugu