1. Patchew
  2. Libvirt
  3. View series

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

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

Patchew 2.1-devel-0870f84

© 2016 - 2023 Red Hat, Inc.