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 <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;

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(&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);

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(&version, tmp, true) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse cloud-hypervisor version: %1$s"), tmp);

Jirka