Re: [libvirt PATCH v3] ch: Fix cloud-hypervisor version processing
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
> >---
> >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 * 100) + (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(_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.. */
> >-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(, 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
On Thu, Sep 07, 2023 at 09:02:44AM +0200, Jiri Denemark wrote:
> On Wed, Sep 06, 2023 at 10:50:30 -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
> > ---
> > 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 * 100) + (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;
>
> As Daniel pointed out in previous version this g_autofree and
> g_steal_pointer below are useless as there is no path that actually free
> the string here.
>
Thanks Jirka, I expected "autofree" attribute to be carried to "tmp" in the
calling method,
which would be freed when tmp goes out of scope. So, I implemented it this way.
With some experimentation, I confirmed the correct behavior.
I will fix this in my next version.
> > +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.
>
> Our coding style document says we prefer /* ... */ comments.
>
> > +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(_version);
> > +
> > +}
> > int
> > chExtractVersion(virCHDriver *driver)
> > {
> > @@ -193,13 +224,20 @@ chExtractVersion(virCHDriver *driver)
> >
> > tmp = help;
> >
> > -/* expected format: cloud-hypervisor v.. */
> > -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);
>
> So chPreProcessVersionString returns an allocated string that will never
> be freed because you assign it to tmp that's only used to point inside
> other buffers and thus is (correctly) not freed anywhere. You need to
> make sure the pointer returned here is freed before chExtractVersion
> exits.
Understood.
>
> > +VIR_DEBUG("Cloud-Hypervisor version detected: %s", tmp);
> > +
> > if (virStringParseVersion(, tmp, true) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("Unable to parse cloud-hypervisor version:
> > %1$s"), tmp);
>
> Jirka
Re: [libvirt PATCH v3] ch: Fix cloud-hypervisor version processing
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
---
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 * 100) + (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(_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.. */
-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(, tmp, true) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to parse cloud-hypervisor version: %1$s"),
tmp);
--
2.41.0
signature.asc
Description: PGP signature
