1. Patchew
  2. Libvirt
  3. View series

[libvirt PATCH v3] ch: Fix cloud-hypervisor version processing

Praveen K Paladugu posted 1 patch 7 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20230906155030.21298-1-prapal@linux.microsoft.com
There is a newer version of this series
src/ch/ch_conf.c | 42 ++++++++++++++++++++++++++++++++++++++++--
1 file changed, 40 insertions(+), 2 deletions(-)
[libvirt PATCH v3] ch: Fix cloud-hypervisor version processing
Posted by Praveen K Paladugu 7 months, 3 weeks ago 
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
Re: [libvirt PATCH v3] ch: Fix cloud-hypervisor version processing
Posted by Martin Kletzander 7 months, 3 weeks ago 
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
>
Re: [libvirt PATCH v3] ch: Fix cloud-hypervisor version processing
Posted by Praveen Paladugu 7 months, 3 weeks ago 
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
> >

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.