[PATCH 7/8] hyperv: implement connectGetVersion

[Pino Toscano]

On Thursday, 1 October 2020 23:47:16 CEST mcoleman at datto.com wrote:
> From: Matt Coleman <matt at datto.com>
> 
> Hyper-V version numbers are not compatible with the encoding in
> virParseVersionString():
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virutil.c#L246
> 
> For example, the Windows Server 2016 Hyper-V version is 10.0.14393: its
> micro is over 14 times larger than the encoding allows.
> 
> This commit repacks the Hyper-V version number in order to preserve all
> of the digits. The major and minor are concatenated (with minor zero-
> padded to two digits) to form the repacked major value. This works
> because Microsoft's major and minor versions numbers are unlikely to
> exceed 99. The repacked minor value is derived from the digits in the
> thousands, ten-thousands, and hundred-thousands places of Hyper-V's
> micro. The repacked micro is derived from the digits in the ones, tens,
> and hundreds places of Hyper-V's micro.
> [...]
> +    /*
> +     * Mangle the version to work with virParseVersionString() while retaining all the digits.
> +     *
> +     * For example...
> +     * 2008:      6.0.6001     =>   600.6.1
> +     * 2008 R2:   6.1.7600     =>   601.7.600
> +     * 2012:      6.2.9200     =>   602.9.200
> +     * 2012 R2:   6.3.9600     =>   603.9.600
> +     * 2016:      10.0.14393   =>   1000.14.393
> +     * 2019:      10.0.17763   =>   1000.17.763
> +     */

IMHO these explanations should be documented in the Hyper-V driver
page: docs/drvhyperv.html.in. This way, users know about the different
value returned by virConnectGetVersion() by the Hyper-V driver, and
can unmangle it to get the real Hyper-V version.

> +    if (virStrToLong_ui(os->data.common->Version, &suffix, 10, &major) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                      _("Could not parse major from '%s'"),
> +                      os->data.common->Version);
> +        goto cleanup;
> +    }
> +
> +    if (virStrToLong_ui(suffix + 1, &suffix, 10, &minor) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                      _("Could not parse minor from '%s'"),
> +                      os->data.common->Version);
> +        goto cleanup;
> +    }
> +
> +    if (virStrToLong_ui(suffix + 1, NULL, 10, &micro) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                      _("Could not parse micro from '%s'"),
> +                      os->data.common->Version);
> +        goto cleanup;
> +    }
> +
> +    if (major > 99 || minor > 99 || micro > 999999) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not produce mangled version number from '%s'"),
> +                       os->data.common->Version);
> +        goto cleanup;
> +    }

I'd move the parsing code to an own helper, similar to
virParseVersionString():

  int
  hypervParseVersionString(const char *str, unsigned int *major,
                           unsigned int *minor, unsigned int *micro)

without virReportError() in it; then use a single virReportError() for
hypervParseVersionString() failure.

> +
> +    virBufferAsprintf(&mangled_version, "%d%02d.%d.%d", major, minor,
> +                      micro > 999 ? micro / 1000 : 0,
> +                      micro > 999 ? micro % 1000 : micro);
> +
> +    /* Parse version string to long */
> +    if (virParseVersionString(virBufferCurrentContent(&mangled_version), version, true) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not parse version number from '%s'"),
> +                       os->data.common->Version);
> +        goto cleanup;
> +    }

Considering the major, minor and micro parts of the version string are
already broken out here, why not directly encode the version number as
long? It seems a forced roundtrip to format it as string, and parse it
again.

> diff --git a/src/hyperv/hyperv_wmi_generator.input b/src/hyperv/hyperv_wmi_generator.input
> index 77543cf6ef..bbca550790 100644
> --- a/src/hyperv/hyperv_wmi_generator.input
> +++ b/src/hyperv/hyperv_wmi_generator.input
> @@ -673,7 +673,7 @@ class Win32_OperatingSystem
>      string   CSCreationClassName
>      string   CSDVersion
>      string   CSName
> -    uint16   CurrentTimeZone
> +    int16    CurrentTimeZone
>      boolean  DataExecutionPrevention_Available
>      boolean  DataExecutionPrevention_32BitApplications
>      boolean  DataExecutionPrevention_Drivers

This seems unrelated to the patch? Or it is needed because this class
is used by the hypervGetWmiClass(Win32_OperatingSystem, ...) call, and
the field needed to be fixed?

I'd split it as separate commit before this one.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201002/3c713cde/attachment-0001.sig>