On Thursday, 1 October 2020 23:47:16 CEST mcoleman@xxxxxxxxx wrote: > From: Matt Coleman <matt@xxxxxxxxx> > > 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, µ) < 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
Attachment:
signature.asc
Description: This is a digitally signed message part.
