[PATCH 0/8] hyperv: implement new APIs & more
From: Matt Coleman <matt@datto.com> These patches fix a couple bugs, consolidate duplicate code, and implement several APIs. Currently, some interactions with Hyper-V systems fail when the system is not configured for the "en-US" locale. Additionally, some CPU names also contain the clock frequency, making it too long for _virNodeInfo.model. The first two patches fix these bugs. The second two patches clean up the code a little: one moves repeated operations into new helper functions; the other replaces the generic "get WMI class list" functions with a macro. The last four patches implement the following APIs in the Hyper-V driver: * virConnectGetCapabilities() * virConnectGetMaxVcpus() * virConnectGetVersion() * virDomainGetAutostart() Matt Coleman (8): hyperv: make Msvm_ComputerSystem WQL queries locale agnostic hyperv: fix nodeGetInfo failures caused by long CPU names hyperv: break out common lookups into separate functions hyperv: replace generic WMI class list helpers with a macro hyperv: implement connectGetCapabilities hyperv: implement connectGetMaxVcpus hyperv: implement connectGetVersion hyperv: implement domainGetAutostart NEWS.rst | 10 + src/hyperv/hyperv_driver.c | 691 ++++++++++++++++++-------- src/hyperv/hyperv_private.h | 2 + src/hyperv/hyperv_wmi.c | 87 +--- src/hyperv/hyperv_wmi.h | 34 +- src/hyperv/hyperv_wmi_classes.h | 4 +- src/hyperv/hyperv_wmi_generator.input | 2 +- 7 files changed, 524 insertions(+), 306 deletions(-) -- 2.27.0
From: Matt Coleman <matt@datto.com> There are two specific WQL queries we're using to get either a list of virtual machines or the hypervisor host itself from Msvm_ComputerSystem. Those queries rely on filtering results based on the "Description" field. Since the "Description" field is locale sensitive, the queries will fail if the Windows host is using a language pack. While the WSMAN spec allows the client to set the requested locale (and it is supported since openwsman 2.6.x), the Windows WinRM service does not respect this setting: it returns non-English strings despite the WSMAN request properly setting the locale to 'en-US'. Therefore, this patch changes the WQL query to make use of the "__SERVER" field to stop relying on English strings in queries and side step the issue. Co-authored-by: Dawid Zamirski <dzamirski@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- NEWS.rst | 4 ++++ src/hyperv/hyperv_wmi_classes.h | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/NEWS.rst b/NEWS.rst index 7663cf4208..5c9ab4a964 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,10 @@ v6.9.0 (unreleased) * **Bug fixes** + * hyperv: ensure WQL queries work in all locales + + Relying on the "Description" field caused queries to fail on non-"en-US" + systems. The queries have been updated to avoid using localized strings. v6.8.0 (2020-10-01) =================== diff --git a/src/hyperv/hyperv_wmi_classes.h b/src/hyperv/hyperv_wmi_classes.h index a19b6a656d..7465684d6e 100644 --- a/src/hyperv/hyperv_wmi_classes.h +++ b/src/hyperv/hyperv_wmi_classes.h @@ -44,10 +44,10 @@ */ #define MSVM_COMPUTERSYSTEM_WQL_VIRTUAL \ - "Description = \"Microsoft Virtual Machine\" " + "Name != __SERVER " #define MSVM_COMPUTERSYSTEM_WQL_PHYSICAL \ - "Description = \"Microsoft Hosting Computer System\" " + "Name = __SERVER " #define MSVM_COMPUTERSYSTEM_WQL_ACTIVE \ "(EnabledState != 0 and EnabledState != 3 and EnabledState != 32769) " -- 2.27.0
From: Matt Coleman <matt@datto.com> Some CPU model names were too long for _virNodeInfo.model. For example: Intel Xeon CPU E5-2620 v2 @ 2.10GHz This commit removes the clock frequency suffix. Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index b57325f2a5..9bbc2f67fb 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -285,6 +285,10 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info) } else if (STRPREFIX(tmp, "(TM)")) { memmove(tmp, tmp + 4, strlen(tmp + 4) + 1); continue; + } else if (STRPREFIX(tmp, " @ ")) { + /* Remove " @ X.YZGHz" from the end. */ + *tmp = '\0'; + break; } ++tmp; -- 2.27.0
From: Matt Coleman <matt@datto.com> This eliminates some duplicate code and simplifies the driver functions. Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 426 ++++++++++++++++++++----------------- 1 file changed, 228 insertions(+), 198 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 9bbc2f67fb..d883fd616c 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -41,6 +41,207 @@ VIR_LOG_INIT("hyperv.hyperv_driver"); +/* + * WMI utility functions + * + * wrapper functions for commonly-accessed WMI objects and interfaces. + */ + +static int +hypervGetProcessorsByName(hypervPrivate *priv, const char *name, + Win32_Processor **processorList) +{ + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + virBufferEscapeSQL(&query, + "ASSOCIATORS OF {Win32_ComputerSystem.Name=\"%s\"} " + "WHERE AssocClass = Win32_ComputerSystemProcessor " + "ResultClass = Win32_Processor", + name); + + if (hypervGetWin32ProcessorList(priv, &query, processorList) < 0 || !processorList) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not look up processor(s) on '%s'"), + name); + return -1; + } + + return 0; +} + +static int +hypervGetActiveVirtualSystemList(hypervPrivate *priv, + Msvm_ComputerSystem **computerSystemList) +{ + g_auto(virBuffer) query = { g_string_new(MSVM_COMPUTERSYSTEM_WQL_SELECT + "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL + "AND " MSVM_COMPUTERSYSTEM_WQL_ACTIVE), 0 }; + + if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not look up active virtual machines")); + return -1; + } + + return 0; +} + +/* gets all the vms including the ones that are marked inactive. */ +static int +hypervGetInactiveVirtualSystemList(hypervPrivate *priv, + Msvm_ComputerSystem **computerSystemList) +{ + g_auto(virBuffer) query = { g_string_new(MSVM_COMPUTERSYSTEM_WQL_SELECT + "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL + "AND " MSVM_COMPUTERSYSTEM_WQL_INACTIVE), 0 }; + + if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not look up inactive virtual machines")); + return -1; + } + + return 0; +} + +static int +hypervGetPhysicalSystemList(hypervPrivate *priv, + Win32_ComputerSystem **computerSystemList) +{ + g_auto(virBuffer) query = { g_string_new(WIN32_COMPUTERSYSTEM_WQL_SELECT), 0 }; + + if (hypervGetWin32ComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not look up Win32_ComputerSystem")); + return -1; + } + + return 0; +} + +static int +hypervGetVirtualSystemByID(hypervPrivate *priv, int id, + Msvm_ComputerSystem **computerSystemList) +{ + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&query, + MSVM_COMPUTERSYSTEM_WQL_SELECT + "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL + "AND ProcessID = %d", + id); + + if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not look up virtual system with ID %d"), id); + return -1; + } + + return 0; +} + +static int +hypervGetVirtualSystemByUUID(hypervPrivate *priv, const char *uuid, + Msvm_ComputerSystem **computerSystemList) +{ + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + virBufferEscapeSQL(&query, + MSVM_COMPUTERSYSTEM_WQL_SELECT + "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL + "AND Name = \"%s\"", + uuid); + + if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not look up virtual system with UUID '%s'"), uuid); + return -1; + } + + return 0; +} + + +static int +hypervGetVirtualSystemByName(hypervPrivate *priv, const char *name, + Msvm_ComputerSystem **computerSystemList) +{ + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + virBufferEscapeSQL(&query, + MSVM_COMPUTERSYSTEM_WQL_SELECT + "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL + "AND ElementName = \"%s\"", + name); + + if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not look up virtual system named '%s'"), name); + return -1; + } + + return 0; +} + +static int +hypervGetVSSDFromUUID(hypervPrivate *priv, const char *uuid, + Msvm_VirtualSystemSettingData **data) +{ + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + virBufferEscapeSQL(&query, + "ASSOCIATORS OF {Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\",Name=\"%s\"} " + "WHERE AssocClass = Msvm_SettingsDefineState " + "ResultClass = Msvm_VirtualSystemSettingData", + uuid); + + if (hypervGetMsvmVirtualSystemSettingDataList(priv, &query, data) < 0 || !*data) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not look up virtual system setting data with UUID '%s'"), + uuid); + return -1; + } + + return 0; +} + +static int +hypervGetProcSDByVSSDInstanceId(hypervPrivate *priv, const char *id, + Msvm_ProcessorSettingData **data) +{ + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + virBufferEscapeSQL(&query, + "ASSOCIATORS OF {Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} " + "WHERE AssocClass = Msvm_VirtualSystemSettingDataComponent " + "ResultClass = Msvm_ProcessorSettingData", + id); + + if (hypervGetMsvmProcessorSettingDataList(priv, &query, data) < 0 || !*data) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not look up processor setting data with virtual system instance ID '%s'"), + id); + return -1; + } + + return 0; +} + +static int +hypervGetMemSDByVSSDInstanceId(hypervPrivate *priv, const char *id, + Msvm_MemorySettingData **data) +{ + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + virBufferEscapeSQL(&query, + "ASSOCIATORS OF {Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} " + "WHERE AssocClass = Msvm_VirtualSystemSettingDataComponent " + "ResultClass = Msvm_MemorySettingData", + id); + + if (hypervGetMsvmMemorySettingDataList(priv, &query, data) < 0 || !*data) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not look up memory setting data with virtual system instance ID '%s'"), + id); + return -1; + } + + return 0; +} + + + +/* + * Driver functions + */ + static void hypervFreePrivate(hypervPrivate **priv) { @@ -203,21 +404,11 @@ hypervConnectGetHostname(virConnectPtr conn) { char *hostname = NULL; hypervPrivate *priv = conn->privateData; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Win32_ComputerSystem *computerSystem = NULL; - virBufferAddLit(&query, WIN32_COMPUTERSYSTEM_WQL_SELECT); - - if (hypervGetWin32ComputerSystemList(priv, &query, &computerSystem) < 0) + if (hypervGetPhysicalSystemList(priv, &computerSystem) < 0) goto cleanup; - if (computerSystem == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not lookup %s"), - "Win32_ComputerSystem"); - goto cleanup; - } - hostname = g_strdup(computerSystem->data.common->DNSHostName); cleanup: @@ -233,7 +424,6 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info) { int result = -1; hypervPrivate *priv = conn->privateData; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Win32_ComputerSystem *computerSystem = NULL; Win32_Processor *processorList = NULL; Win32_Processor *processor = NULL; @@ -241,34 +431,11 @@ hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info) memset(info, 0, sizeof(*info)); - virBufferAddLit(&query, WIN32_COMPUTERSYSTEM_WQL_SELECT); - - /* Get Win32_ComputerSystem */ - if (hypervGetWin32ComputerSystemList(priv, &query, &computerSystem) < 0) + if (hypervGetPhysicalSystemList(priv, &computerSystem) < 0) goto cleanup; - if (computerSystem == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not lookup %s"), - "Win32_ComputerSystem"); - goto cleanup; - } - - /* Get Win32_Processor list */ - virBufferEscapeSQL(&query, - "associators of " - "{Win32_ComputerSystem.Name=\"%s\"} " - "where AssocClass = Win32_ComputerSystemProcessor " - "ResultClass = Win32_Processor", - computerSystem->data.common->Name); - - if (hypervGetWin32ProcessorList(priv, &query, &processorList) < 0) - goto cleanup; - - if (processorList == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not lookup %s"), - "Win32_Processor"); + if (hypervGetProcessorsByName(priv, computerSystem->data.common->Name, + &processorList) < 0) { goto cleanup; } @@ -332,7 +499,6 @@ hypervConnectListDomains(virConnectPtr conn, int *ids, int maxids) { bool success = false; hypervPrivate *priv = conn->privateData; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystemList = NULL; Msvm_ComputerSystem *computerSystem = NULL; int count = 0; @@ -340,16 +506,8 @@ hypervConnectListDomains(virConnectPtr conn, int *ids, int maxids) if (maxids == 0) return 0; - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); - virBufferAddLit(&query, "where "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); - virBufferAddLit(&query, "and "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_ACTIVE); - - if (hypervGetMsvmComputerSystemList(priv, &query, - &computerSystemList) < 0) { + if (hypervGetActiveVirtualSystemList(priv, &computerSystemList) < 0) goto cleanup; - } for (computerSystem = computerSystemList; computerSystem != NULL; computerSystem = computerSystem->next) { @@ -374,21 +532,12 @@ hypervConnectNumOfDomains(virConnectPtr conn) { bool success = false; hypervPrivate *priv = conn->privateData; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystemList = NULL; Msvm_ComputerSystem *computerSystem = NULL; int count = 0; - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); - virBufferAddLit(&query, "where "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); - virBufferAddLit(&query, "and "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_ACTIVE); - - if (hypervGetMsvmComputerSystemList(priv, &query, - &computerSystemList) < 0) { + if (hypervGetActiveVirtualSystemList(priv, &computerSystemList) < 0) goto cleanup; - } for (computerSystem = computerSystemList; computerSystem != NULL; computerSystem = computerSystem->next) { @@ -410,18 +559,9 @@ hypervDomainLookupByID(virConnectPtr conn, int id) { virDomainPtr domain = NULL; hypervPrivate *priv = conn->privateData; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystem = NULL; - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); - virBufferAddLit(&query, "where "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); - virBufferAsprintf(&query, "and ProcessID = %d", id); - - if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0) - goto cleanup; - - if (computerSystem == NULL) { + if (hypervGetVirtualSystemByID(priv, id, &computerSystem) < 0) { virReportError(VIR_ERR_NO_DOMAIN, _("No domain with ID %d"), id); goto cleanup; } @@ -442,20 +582,11 @@ hypervDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainPtr domain = NULL; hypervPrivate *priv = conn->privateData; char uuid_string[VIR_UUID_STRING_BUFLEN]; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystem = NULL; virUUIDFormat(uuid, uuid_string); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); - virBufferAddLit(&query, "where "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); - virBufferEscapeSQL(&query, "and Name = \"%s\"", uuid_string); - - if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0) - goto cleanup; - - if (computerSystem == NULL) { + if (hypervGetVirtualSystemByUUID(priv, uuid_string, &computerSystem) < 0) { virReportError(VIR_ERR_NO_DOMAIN, _("No domain with UUID %s"), uuid_string); goto cleanup; @@ -476,18 +607,9 @@ hypervDomainLookupByName(virConnectPtr conn, const char *name) { virDomainPtr domain = NULL; hypervPrivate *priv = conn->privateData; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystem = NULL; - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); - virBufferAddLit(&query, "where "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); - virBufferEscapeSQL(&query, "and ElementName = \"%s\"", name); - - if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0) - goto cleanup; - - if (computerSystem == NULL) { + if (hypervGetVirtualSystemByName(priv, name, &computerSystem) < 0) { virReportError(VIR_ERR_NO_DOMAIN, _("No domain with name %s"), name); goto cleanup; @@ -615,7 +737,6 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) int result = -1; hypervPrivate *priv = domain->conn->privateData; char uuid_string[VIR_UUID_STRING_BUFLEN]; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystem = NULL; Msvm_VirtualSystemSettingData *virtualSystemSettingData = NULL; Msvm_ProcessorSettingData *processorSettingData = NULL; @@ -629,21 +750,8 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) goto cleanup; - /* Get Msvm_VirtualSystemSettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\"," - "Name=\"%s\"} " - "where AssocClass = Msvm_SettingsDefineState " - "ResultClass = Msvm_VirtualSystemSettingData", - uuid_string); - - if (hypervGetMsvmVirtualSystemSettingDataList(priv, &query, - &virtualSystemSettingData) < 0) { - goto cleanup; - } - - if (virtualSystemSettingData == NULL) { + if (hypervGetVSSDFromUUID(priv, uuid_string, + &virtualSystemSettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_VirtualSystemSettingData", @@ -651,20 +759,9 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) goto cleanup; } - /* Get Msvm_ProcessorSettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} " - "where AssocClass = Msvm_VirtualSystemSettingDataComponent " - "ResultClass = Msvm_ProcessorSettingData", - virtualSystemSettingData->data.common->InstanceID); - - if (hypervGetMsvmProcessorSettingDataList(priv, &query, - &processorSettingData) < 0) { - goto cleanup; - } - - if (processorSettingData == NULL) { + if (hypervGetProcSDByVSSDInstanceId(priv, + virtualSystemSettingData->data.common->InstanceID, + &processorSettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_ProcessorSettingData", @@ -672,21 +769,9 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) goto cleanup; } - /* Get Msvm_MemorySettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} " - "where AssocClass = Msvm_VirtualSystemSettingDataComponent " - "ResultClass = Msvm_MemorySettingData", - virtualSystemSettingData->data.common->InstanceID); - - if (hypervGetMsvmMemorySettingDataList(priv, &query, - &memorySettingData) < 0) { - goto cleanup; - } - - - if (memorySettingData == NULL) { + if (hypervGetMemSDByVSSDInstanceId(priv, + virtualSystemSettingData->data.common->InstanceID, + &memorySettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_MemorySettingData", @@ -749,7 +834,6 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) hypervPrivate *priv = domain->conn->privateData; virDomainDefPtr def = NULL; char uuid_string[VIR_UUID_STRING_BUFLEN]; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystem = NULL; Msvm_VirtualSystemSettingData *virtualSystemSettingData = NULL; Msvm_ProcessorSettingData *processorSettingData = NULL; @@ -766,21 +850,8 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) goto cleanup; - /* Get Msvm_VirtualSystemSettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\"," - "Name=\"%s\"} " - "where AssocClass = Msvm_SettingsDefineState " - "ResultClass = Msvm_VirtualSystemSettingData", - uuid_string); - - if (hypervGetMsvmVirtualSystemSettingDataList(priv, &query, - &virtualSystemSettingData) < 0) { - goto cleanup; - } - - if (virtualSystemSettingData == NULL) { + if (hypervGetVSSDFromUUID(priv, uuid_string, + &virtualSystemSettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_VirtualSystemSettingData", @@ -788,20 +859,9 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) goto cleanup; } - /* Get Msvm_ProcessorSettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} " - "where AssocClass = Msvm_VirtualSystemSettingDataComponent " - "ResultClass = Msvm_ProcessorSettingData", - virtualSystemSettingData->data.common->InstanceID); - - if (hypervGetMsvmProcessorSettingDataList(priv, &query, - &processorSettingData) < 0) { - goto cleanup; - } - - if (processorSettingData == NULL) { + if (hypervGetProcSDByVSSDInstanceId(priv, + virtualSystemSettingData->data.common->InstanceID, + &processorSettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_ProcessorSettingData", @@ -809,21 +869,9 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) goto cleanup; } - /* Get Msvm_MemorySettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} " - "where AssocClass = Msvm_VirtualSystemSettingDataComponent " - "ResultClass = Msvm_MemorySettingData", - virtualSystemSettingData->data.common->InstanceID); - - if (hypervGetMsvmMemorySettingDataList(priv, &query, - &memorySettingData) < 0) { - goto cleanup; - } - - - if (memorySettingData == NULL) { + if (hypervGetMemSDByVSSDInstanceId(priv, + virtualSystemSettingData->data.common->InstanceID, + &memorySettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_MemorySettingData", @@ -909,7 +957,6 @@ hypervConnectListDefinedDomains(virConnectPtr conn, char **const names, int maxn { bool success = false; hypervPrivate *priv = conn->privateData; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystemList = NULL; Msvm_ComputerSystem *computerSystem = NULL; int count = 0; @@ -918,16 +965,8 @@ hypervConnectListDefinedDomains(virConnectPtr conn, char **const names, int maxn if (maxnames == 0) return 0; - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); - virBufferAddLit(&query, "where "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); - virBufferAddLit(&query, "and "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_INACTIVE); - - if (hypervGetMsvmComputerSystemList(priv, &query, - &computerSystemList) < 0) { + if (hypervGetInactiveVirtualSystemList(priv, &computerSystemList) < 0) goto cleanup; - } for (computerSystem = computerSystemList; computerSystem != NULL; computerSystem = computerSystem->next) { @@ -961,21 +1000,12 @@ hypervConnectNumOfDefinedDomains(virConnectPtr conn) { bool success = false; hypervPrivate *priv = conn->privateData; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystemList = NULL; Msvm_ComputerSystem *computerSystem = NULL; int count = 0; - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); - virBufferAddLit(&query, "where "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); - virBufferAddLit(&query, "and "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_INACTIVE); - - if (hypervGetMsvmComputerSystemList(priv, &query, - &computerSystemList) < 0) { + if (hypervGetInactiveVirtualSystemList(priv, &computerSystemList) < 0) goto cleanup; - } for (computerSystem = computerSystemList; computerSystem != NULL; computerSystem = computerSystem->next) { -- 2.27.0
On Thursday, 1 October 2020 23:47:12 CEST mcoleman@datto.com wrote:
+static int +hypervGetProcessorsByName(hypervPrivate *priv, const char *name, + Win32_Processor **processorList) +{ + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + virBufferEscapeSQL(&query, + "ASSOCIATORS OF {Win32_ComputerSystem.Name=\"%s\"} " + "WHERE AssocClass = Win32_ComputerSystemProcessor " + "ResultClass = Win32_Processor", + name); + + if (hypervGetWin32ProcessorList(priv, &query, processorList) < 0 || !processorList) {
This line seems too long, and it is easy to not notice the second check when not using work wrapping. I suggest to split it in two lines, i.e.: if (hypervGetWin32ProcessorList(priv, &query, processorList) < 0 || !processorList) { The same applies for all the other checks in the new functions of this patch.
+static int +hypervGetActiveVirtualSystemList(hypervPrivate *priv, + Msvm_ComputerSystem **computerSystemList) +{ + g_auto(virBuffer) query = { g_string_new(MSVM_COMPUTERSYSTEM_WQL_SELECT + "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL + "AND " MSVM_COMPUTERSYSTEM_WQL_ACTIVE), 0 }; + + if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not look up active virtual machines"));
This virReportError() needs to be split in multiple lines; the same applies to the other virReportError()s.
@@ -410,18 +559,9 @@ hypervDomainLookupByID(virConnectPtr conn, int id) { virDomainPtr domain = NULL; hypervPrivate *priv = conn->privateData; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystem = NULL;
- virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); - virBufferAddLit(&query, "where "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); - virBufferAsprintf(&query, "and ProcessID = %d", id); - - if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0) - goto cleanup; - - if (computerSystem == NULL) { + if (hypervGetVirtualSystemByID(priv, id, &computerSystem) < 0) { virReportError(VIR_ERR_NO_DOMAIN, _("No domain with ID %d"), id); goto cleanup;
Note that now hypervGetVirtualSystemByID() issues VIR_ERR_INTERNAL_ERROR in case &computerSystem / *computerSystemList is null, instead of VIR_ERR_NO_DOMAIN. Shouldn't this still be able to explicitly report when the requested domain does not exist?
@@ -442,20 +582,11 @@ hypervDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainPtr domain = NULL; hypervPrivate *priv = conn->privateData; char uuid_string[VIR_UUID_STRING_BUFLEN]; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystem = NULL;
virUUIDFormat(uuid, uuid_string);
- virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); - virBufferAddLit(&query, "where "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); - virBufferEscapeSQL(&query, "and Name = \"%s\"", uuid_string); - - if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0) - goto cleanup; - - if (computerSystem == NULL) { + if (hypervGetVirtualSystemByUUID(priv, uuid_string, &computerSystem) < 0) { virReportError(VIR_ERR_NO_DOMAIN, _("No domain with UUID %s"), uuid_string);
Ditto.
@@ -476,18 +607,9 @@ hypervDomainLookupByName(virConnectPtr conn, const char *name) { virDomainPtr domain = NULL; hypervPrivate *priv = conn->privateData; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystem = NULL;
- virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_SELECT); - virBufferAddLit(&query, "where "); - virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); - virBufferEscapeSQL(&query, "and ElementName = \"%s\"", name); - - if (hypervGetMsvmComputerSystemList(priv, &query, &computerSystem) < 0) - goto cleanup; - - if (computerSystem == NULL) { + if (hypervGetVirtualSystemByName(priv, name, &computerSystem) < 0) { virReportError(VIR_ERR_NO_DOMAIN, _("No domain with name %s"), name);
Ditto.
@@ -615,7 +737,6 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) int result = -1; hypervPrivate *priv = domain->conn->privateData; char uuid_string[VIR_UUID_STRING_BUFLEN]; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystem = NULL; Msvm_VirtualSystemSettingData *virtualSystemSettingData = NULL; Msvm_ProcessorSettingData *processorSettingData = NULL; @@ -629,21 +750,8 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) goto cleanup;
- /* Get Msvm_VirtualSystemSettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\"," - "Name=\"%s\"} " - "where AssocClass = Msvm_SettingsDefineState " - "ResultClass = Msvm_VirtualSystemSettingData", - uuid_string); - - if (hypervGetMsvmVirtualSystemSettingDataList(priv, &query, - &virtualSystemSettingData) < 0) { - goto cleanup; - } - - if (virtualSystemSettingData == NULL) { + if (hypervGetVSSDFromUUID(priv, uuid_string, + &virtualSystemSettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_VirtualSystemSettingData",
Considering the new hypervGetVSSDFromUUID() already uses virReportError() on failure, this virReportError() above seems not needed anymore.
@@ -651,20 +759,9 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) goto cleanup; }
- /* Get Msvm_ProcessorSettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} " - "where AssocClass = Msvm_VirtualSystemSettingDataComponent " - "ResultClass = Msvm_ProcessorSettingData", - virtualSystemSettingData->data.common->InstanceID); - - if (hypervGetMsvmProcessorSettingDataList(priv, &query, - &processorSettingData) < 0) { - goto cleanup; - } - - if (processorSettingData == NULL) { + if (hypervGetProcSDByVSSDInstanceId(priv, + virtualSystemSettingData->data.common->InstanceID, + &processorSettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_ProcessorSettingData",
Ditto.
@@ -672,21 +769,9 @@ hypervDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) goto cleanup; }
- /* Get Msvm_MemorySettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} " - "where AssocClass = Msvm_VirtualSystemSettingDataComponent " - "ResultClass = Msvm_MemorySettingData", - virtualSystemSettingData->data.common->InstanceID); - - if (hypervGetMsvmMemorySettingDataList(priv, &query, - &memorySettingData) < 0) { - goto cleanup; - } - - - if (memorySettingData == NULL) { + if (hypervGetMemSDByVSSDInstanceId(priv, + virtualSystemSettingData->data.common->InstanceID, + &memorySettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_MemorySettingData",
Ditto.
@@ -749,7 +834,6 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) hypervPrivate *priv = domain->conn->privateData; virDomainDefPtr def = NULL; char uuid_string[VIR_UUID_STRING_BUFLEN]; - g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; Msvm_ComputerSystem *computerSystem = NULL; Msvm_VirtualSystemSettingData *virtualSystemSettingData = NULL; Msvm_ProcessorSettingData *processorSettingData = NULL; @@ -766,21 +850,8 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) goto cleanup;
- /* Get Msvm_VirtualSystemSettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_ComputerSystem.CreationClassName=\"Msvm_ComputerSystem\"," - "Name=\"%s\"} " - "where AssocClass = Msvm_SettingsDefineState " - "ResultClass = Msvm_VirtualSystemSettingData", - uuid_string); - - if (hypervGetMsvmVirtualSystemSettingDataList(priv, &query, - &virtualSystemSettingData) < 0) { - goto cleanup; - } - - if (virtualSystemSettingData == NULL) { + if (hypervGetVSSDFromUUID(priv, uuid_string, + &virtualSystemSettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_VirtualSystemSettingData",
Ditto.
@@ -788,20 +859,9 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) goto cleanup; }
- /* Get Msvm_ProcessorSettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} " - "where AssocClass = Msvm_VirtualSystemSettingDataComponent " - "ResultClass = Msvm_ProcessorSettingData", - virtualSystemSettingData->data.common->InstanceID); - - if (hypervGetMsvmProcessorSettingDataList(priv, &query, - &processorSettingData) < 0) { - goto cleanup; - } - - if (processorSettingData == NULL) { + if (hypervGetProcSDByVSSDInstanceId(priv, + virtualSystemSettingData->data.common->InstanceID, + &processorSettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_ProcessorSettingData",
Ditto.
@@ -809,21 +869,9 @@ hypervDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) goto cleanup; }
- /* Get Msvm_MemorySettingData */ - virBufferEscapeSQL(&query, - "associators of " - "{Msvm_VirtualSystemSettingData.InstanceID=\"%s\"} " - "where AssocClass = Msvm_VirtualSystemSettingDataComponent " - "ResultClass = Msvm_MemorySettingData", - virtualSystemSettingData->data.common->InstanceID); - - if (hypervGetMsvmMemorySettingDataList(priv, &query, - &memorySettingData) < 0) { - goto cleanup; - } - - - if (memorySettingData == NULL) { + if (hypervGetMemSDByVSSDInstanceId(priv, + virtualSystemSettingData->data.common->InstanceID, + &memorySettingData) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not lookup %s for domain %s"), "Msvm_MemorySettingData",
Ditto. -- Pino Toscano
On Oct 2, 2020, at 2:25 AM, Pino Toscano <ptoscano@redhat.com> wrote:
Note that now hypervGetVirtualSystemByID() issues VIR_ERR_INTERNAL_ERROR in case &computerSystem / *computerSystemList is null, instead of VIR_ERR_NO_DOMAIN. Shouldn't this still be able to explicitly report when the requested domain does not exist?
I’ll split it up into two conditionals. Would you prefer to see the NULL check that produces the VIR_ERR_NO_DOMAIN error in the new helper function (hypervGetVirtualSystemByID) or its caller (hypervDomainLookupByID)? I’ll also change the code to report VIR_ERR_OPERATION_FAILED instead of VIR_ERR_INTERNAL_ERROR for WS-MAN failures. I had considered VIR_ERR_RPC, but decided to go with VIR_ERR_OPERATION_FAILED since WS-MAN isn’t exactly RPC. Thanks again for all your help. -- Matt
On Friday, 2 October 2020 23:50:22 CEST Matt Coleman wrote:
On Oct 2, 2020, at 2:25 AM, Pino Toscano <ptoscano@redhat.com> wrote:
Note that now hypervGetVirtualSystemByID() issues VIR_ERR_INTERNAL_ERROR in case &computerSystem / *computerSystemList is null, instead of VIR_ERR_NO_DOMAIN. Shouldn't this still be able to explicitly report when the requested domain does not exist?
I’ll split it up into two conditionals. Would you prefer to see the NULL check that produces the VIR_ERR_NO_DOMAIN error in the new helper function (hypervGetVirtualSystemByID) or its caller (hypervDomainLookupByID)?
This depends on the callers: if the logic for checking whether a domain is considered "not already existing" makes sense for all the callers of hypervGetVirtualSystemByID, then I'd say to add it there. -- Pino Toscano
From: Matt Coleman <matt@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 39 +++++++++++------ src/hyperv/hyperv_wmi.c | 87 ++++---------------------------------- src/hyperv/hyperv_wmi.h | 34 +++------------ 3 files changed, 41 insertions(+), 119 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index d883fd616c..4ba226d446 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -47,6 +47,20 @@ VIR_LOG_INIT("hyperv.hyperv_driver"); * wrapper functions for commonly-accessed WMI objects and interfaces. */ +/** + * hypervGetWmiClass: + * @type: the type of the class being retrieved from WMI + * @class: double pointer where the class data will be stored + * + * Retrieve one or more classes from WMI. + * + * The following variables must exist in the caller: + * 1. hypervPrivate *priv + * 2. virBuffer query + */ +#define hypervGetWmiClass(type, class) \ + hypervGetWmiClassList(priv, type ## _WmiInfo, &query, (hypervObject **)class) + static int hypervGetProcessorsByName(hypervPrivate *priv, const char *name, Win32_Processor **processorList) @@ -58,7 +72,7 @@ hypervGetProcessorsByName(hypervPrivate *priv, const char *name, "ResultClass = Win32_Processor", name); - if (hypervGetWin32ProcessorList(priv, &query, processorList) < 0 || !processorList) { + if (hypervGetWmiClass(Win32_Processor, processorList) < 0 || !processorList) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not look up processor(s) on '%s'"), name); @@ -76,7 +90,7 @@ hypervGetActiveVirtualSystemList(hypervPrivate *priv, "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL "AND " MSVM_COMPUTERSYSTEM_WQL_ACTIVE), 0 }; - if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + if (hypervGetWmiClass(Msvm_ComputerSystem, computerSystemList) < 0 || !*computerSystemList) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not look up active virtual machines")); return -1; } @@ -93,7 +107,7 @@ hypervGetInactiveVirtualSystemList(hypervPrivate *priv, "WHERE " MSVM_COMPUTERSYSTEM_WQL_VIRTUAL "AND " MSVM_COMPUTERSYSTEM_WQL_INACTIVE), 0 }; - if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + if (hypervGetWmiClass(Msvm_ComputerSystem, computerSystemList) < 0 || !*computerSystemList) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not look up inactive virtual machines")); return -1; } @@ -107,7 +121,7 @@ hypervGetPhysicalSystemList(hypervPrivate *priv, { g_auto(virBuffer) query = { g_string_new(WIN32_COMPUTERSYSTEM_WQL_SELECT), 0 }; - if (hypervGetWin32ComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + if (hypervGetWmiClass(Win32_ComputerSystem, computerSystemList) < 0 || !*computerSystemList) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not look up Win32_ComputerSystem")); return -1; } @@ -126,7 +140,7 @@ hypervGetVirtualSystemByID(hypervPrivate *priv, int id, "AND ProcessID = %d", id); - if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + if (hypervGetWmiClass(Msvm_ComputerSystem, computerSystemList) < 0 || !*computerSystemList) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not look up virtual system with ID %d"), id); return -1; } @@ -145,7 +159,7 @@ hypervGetVirtualSystemByUUID(hypervPrivate *priv, const char *uuid, "AND Name = \"%s\"", uuid); - if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + if (hypervGetWmiClass(Msvm_ComputerSystem, computerSystemList) < 0 || !*computerSystemList) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not look up virtual system with UUID '%s'"), uuid); return -1; } @@ -165,7 +179,7 @@ hypervGetVirtualSystemByName(hypervPrivate *priv, const char *name, "AND ElementName = \"%s\"", name); - if (hypervGetMsvmComputerSystemList(priv, &query, computerSystemList) < 0 || !*computerSystemList) { + if (hypervGetWmiClass(Msvm_ComputerSystem, computerSystemList) < 0 || !*computerSystemList) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not look up virtual system named '%s'"), name); return -1; } @@ -184,7 +198,7 @@ hypervGetVSSDFromUUID(hypervPrivate *priv, const char *uuid, "ResultClass = Msvm_VirtualSystemSettingData", uuid); - if (hypervGetMsvmVirtualSystemSettingDataList(priv, &query, data) < 0 || !*data) { + if (hypervGetWmiClass(Msvm_VirtualSystemSettingData, data) < 0 || !*data) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not look up virtual system setting data with UUID '%s'"), uuid); @@ -205,7 +219,7 @@ hypervGetProcSDByVSSDInstanceId(hypervPrivate *priv, const char *id, "ResultClass = Msvm_ProcessorSettingData", id); - if (hypervGetMsvmProcessorSettingDataList(priv, &query, data) < 0 || !*data) { + if (hypervGetWmiClass(Msvm_ProcessorSettingData, data) < 0 || !*data) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not look up processor setting data with virtual system instance ID '%s'"), id); @@ -226,7 +240,7 @@ hypervGetMemSDByVSSDInstanceId(hypervPrivate *priv, const char *id, "ResultClass = Msvm_MemorySettingData", id); - if (hypervGetMsvmMemorySettingDataList(priv, &query, data) < 0 || !*data) { + if (hypervGetWmiClass(Msvm_MemorySettingData, data) < 0 || !*data) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not look up memory setting data with virtual system instance ID '%s'"), id); @@ -1279,8 +1293,7 @@ hypervConnectListAllDomains(virConnectPtr conn, } } - if (hypervGetMsvmComputerSystemList(priv, &query, - &computerSystemList) < 0) + if (hypervGetWmiClass(Msvm_ComputerSystem, &computerSystemList) < 0) goto cleanup; if (domains) { @@ -1386,7 +1399,7 @@ hypervDomainSendKey(virDomainPtr domain, unsigned int codeset, "where ResultClass = Msvm_Keyboard", uuid_string); - if (hypervGetMsvmKeyboardList(priv, &query, &keyboard) < 0) + if (hypervGetWmiClass(Msvm_Keyboard, &keyboard) < 0) goto cleanup; translatedKeycodes = g_new0(int, nkeycodes); diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 809f68a844..c1c244868b 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -83,7 +83,7 @@ hypervGetWmiClassInfo(hypervPrivate *priv, hypervWmiClassInfoListPtr list, return -1; } -static int +int hypervGetWmiClassList(hypervPrivate *priv, hypervWmiClassInfoListPtr wmiInfo, virBufferPtr query, hypervObject **wmiClass) { @@ -878,8 +878,8 @@ hypervInvokeMethod(hypervPrivate *priv, hypervInvokeParamsListPtr params, virBufferAddLit(&query, MSVM_CONCRETEJOB_WQL_SELECT); virBufferEscapeSQL(&query, "where InstanceID = \"%s\"", instanceID); - if (hypervGetMsvmConcreteJobList(priv, &query, &job) < 0 - || job == NULL) + if (hypervGetWmiClassList(priv, Msvm_ConcreteJob_WmiInfo, &query, + (hypervObject **)&job) < 0 || job == NULL) goto cleanup; jobState = job->data.common->JobState; @@ -1218,77 +1218,6 @@ hypervReturnCodeToString(int returnCode) -/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * - * Generic "Get WMI class list" helpers - */ - -int -hypervGetMsvmComputerSystemList(hypervPrivate *priv, virBufferPtr query, - Msvm_ComputerSystem **list) -{ - return hypervGetWmiClassList(priv, Msvm_ComputerSystem_WmiInfo, query, - (hypervObject **)list); -} - -int -hypervGetMsvmConcreteJobList(hypervPrivate *priv, virBufferPtr query, - Msvm_ConcreteJob **list) -{ - return hypervGetWmiClassList(priv, Msvm_ConcreteJob_WmiInfo, query, - (hypervObject **)list); -} - -int -hypervGetWin32ComputerSystemList(hypervPrivate *priv, virBufferPtr query, - Win32_ComputerSystem **list) -{ - return hypervGetWmiClassList(priv, Win32_ComputerSystem_WmiInfo, query, - (hypervObject **)list); -} - -int -hypervGetWin32ProcessorList(hypervPrivate *priv, virBufferPtr query, - Win32_Processor **list) -{ - return hypervGetWmiClassList(priv, Win32_Processor_WmiInfo, query, - (hypervObject **)list); -} - -int -hypervGetMsvmVirtualSystemSettingDataList(hypervPrivate *priv, - virBufferPtr query, - Msvm_VirtualSystemSettingData **list) -{ - return hypervGetWmiClassList(priv, Msvm_VirtualSystemSettingData_WmiInfo, query, - (hypervObject **)list); -} - -int -hypervGetMsvmProcessorSettingDataList(hypervPrivate *priv, - virBufferPtr query, - Msvm_ProcessorSettingData **list) -{ - return hypervGetWmiClassList(priv, Msvm_ProcessorSettingData_WmiInfo, query, - (hypervObject **)list); -} - -int -hypervGetMsvmMemorySettingDataList(hypervPrivate *priv, virBufferPtr query, - Msvm_MemorySettingData **list) -{ - return hypervGetWmiClassList(priv, Msvm_MemorySettingData_WmiInfo, query, - (hypervObject **)list); -} - -int hypervGetMsvmKeyboardList(hypervPrivate *priv, virBufferPtr query, - Msvm_Keyboard **list) -{ - return hypervGetWmiClassList(priv, Msvm_Keyboard_WmiInfo, query, - (hypervObject **)list); -} - - - /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * Msvm_ComputerSystem */ @@ -1371,7 +1300,8 @@ hypervInvokeMsvmComputerSystemRequestStateChange(virDomainPtr domain, virBufferAddLit(&query, MSVM_CONCRETEJOB_WQL_SELECT); virBufferAsprintf(&query, "where InstanceID = \"%s\"", instanceID); - if (hypervGetMsvmConcreteJobList(priv, &query, &concreteJob) < 0) + if (hypervGetWmiClassList(priv, Msvm_ConcreteJob_WmiInfo, &query, + (hypervObject **)&concreteJob) < 0) goto cleanup; if (concreteJob == NULL) { @@ -1560,7 +1490,8 @@ hypervMsvmComputerSystemFromDomain(virDomainPtr domain, virBufferAddLit(&query, MSVM_COMPUTERSYSTEM_WQL_VIRTUAL); virBufferAsprintf(&query, "and Name = \"%s\"", uuid_string); - if (hypervGetMsvmComputerSystemList(priv, &query, computerSystem) < 0) + if (hypervGetWmiClassList(priv, Msvm_ComputerSystem_WmiInfo, &query, + (hypervObject **)computerSystem) < 0) return -1; if (*computerSystem == NULL) { @@ -1592,7 +1523,7 @@ hypervGetMsvmVirtualSystemSettingDataFromUUID(hypervPrivate *priv, uuid_string); if (hypervGetWmiClassList(priv, Msvm_VirtualSystemSettingData_WmiInfo, &query, - (hypervObject **)list) < 0 || *list == NULL) + (hypervObject **)list) < 0 || *list == NULL) return -1; return 0; @@ -1617,7 +1548,7 @@ hypervGetMsvmMemorySettingDataFromVSSD(hypervPrivate *priv, vssd_instanceid); if (hypervGetWmiClassList(priv, Msvm_MemorySettingData_WmiInfo, &query, - (hypervObject **)list) < 0 || *list == NULL) + (hypervObject **)list) < 0 || *list == NULL) return -1; return 0; diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h index 74a74e0e09..c493a45691 100644 --- a/src/hyperv/hyperv_wmi.h +++ b/src/hyperv/hyperv_wmi.h @@ -37,9 +37,6 @@ #define MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR \ "CreationClassName=Msvm_VirtualSystemManagementService" -int hypervVerifyResponse(WsManClient *client, WsXmlDocH response, - const char *detail); - /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * @@ -195,43 +192,24 @@ const char *hypervReturnCodeToString(int returnCode); /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * - * Generic "Get WMI class list" helpers + * WMI class list helpers */ -int hypervGetMsvmComputerSystemList(hypervPrivate *priv, virBufferPtr query, - Msvm_ComputerSystem **list); - -int hypervGetMsvmConcreteJobList(hypervPrivate *priv, virBufferPtr query, - Msvm_ConcreteJob **list); - -int hypervGetWin32ComputerSystemList(hypervPrivate *priv, virBufferPtr query, - Win32_ComputerSystem **list); - -int hypervGetWin32ProcessorList(hypervPrivate *priv, virBufferPtr query, - Win32_Processor **list); +int hypervGetWmiClassList(hypervPrivate *priv, + hypervWmiClassInfoListPtr wmiInfo, virBufferPtr query, + hypervObject **wmiClass); -int hypervGetMsvmVirtualSystemSettingDataList(hypervPrivate *priv, - virBufferPtr query, - Msvm_VirtualSystemSettingData **list); +int hypervVerifyResponse(WsManClient *client, WsXmlDocH response, + const char *detail); int hypervGetMsvmVirtualSystemSettingDataFromUUID(hypervPrivate *priv, const char *uuid_string, Msvm_VirtualSystemSettingData **list); -int hypervGetMsvmProcessorSettingDataList(hypervPrivate *priv, - virBufferPtr query, - Msvm_ProcessorSettingData **list); - -int hypervGetMsvmMemorySettingDataList(hypervPrivate *priv, virBufferPtr query, - Msvm_MemorySettingData **list); - int hypervGetMsvmMemorySettingDataFromVSSD(hypervPrivate *priv, const char *vssd_instanceid, Msvm_MemorySettingData **list); -int hypervGetMsvmKeyboardList(hypervPrivate *priv, virBufferPtr query, - Msvm_Keyboard **list); - /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * Msvm_ComputerSystem */ -- 2.27.0
On Thursday, 1 October 2020 23:47:13 CEST mcoleman@datto.com wrote:
diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h index 74a74e0e09..c493a45691 100644 --- a/src/hyperv/hyperv_wmi.h +++ b/src/hyperv/hyperv_wmi.h @@ -37,9 +37,6 @@ #define MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR \ "CreationClassName=Msvm_VirtualSystemManagementService"
-int hypervVerifyResponse(WsManClient *client, WsXmlDocH response, - const char *detail); -
This change seems unrelated to the patch. Can you please split it in an own commit? -- Pino Toscano
On Oct 2, 2020, at 2:26 AM, Pino Toscano <ptoscano@redhat.com> wrote:
This change seems unrelated to the patch. Can you please split it in an own commit?
I originally had the macro in this header and had moved the function next to the other closely-related functions. Since I moved the macro to hyperv_driver.c, it no longer makes sense. Thanks for pointing that out. -- Matt
From: Matt Coleman <matt@datto.com> Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- NEWS.rst | 5 +++ src/hyperv/hyperv_driver.c | 90 +++++++++++++++++++++++++++++++++++++ src/hyperv/hyperv_private.h | 2 + 3 files changed, 97 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 5c9ab4a964..65fa616a03 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -13,6 +13,11 @@ v6.9.0 (unreleased) * **New features** + * hyperv: implement ``virConnectGetCapabilities()`` + + The ``virConnectGetCapabilities()`` API has been implemented in the Hyper-V + driver. + * **Improvements** * **Bug fixes** diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 4ba226d446..b1f84c568f 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -36,6 +36,7 @@ #include "openwsman.h" #include "virstring.h" #include "virkeycode.h" +#include "domain_conf.h" #define VIR_FROM_THIS VIR_FROM_HYPERV @@ -252,6 +253,76 @@ hypervGetMemSDByVSSDInstanceId(hypervPrivate *priv, const char *id, +/* + * API-specific utility functions + */ + +static int +hypervLookupHostSystemBiosUuid(hypervPrivate *priv, unsigned char *uuid) +{ + Win32_ComputerSystemProduct *computerSystem = NULL; + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + int result = -1; + + virBufferAddLit(&query, WIN32_COMPUTERSYSTEMPRODUCT_WQL_SELECT); + if (hypervGetWmiClass(Win32_ComputerSystemProduct, &computerSystem) < 0) + goto cleanup; + + if (virUUIDParse(computerSystem->data.common->UUID, uuid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not parse UUID from string '%s'"), + computerSystem->data.common->UUID); + goto cleanup; + } + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *) computerSystem); + + return result; +} + +static virCapsPtr +hypervCapsInit(hypervPrivate *priv) +{ + virCapsPtr caps = NULL; + virCapsGuestPtr guest = NULL; + + caps = virCapabilitiesNew(VIR_ARCH_X86_64, 1, 1); + + if (!caps) + return NULL; + + if (hypervLookupHostSystemBiosUuid(priv, caps->host.host_uuid) < 0) + goto error; + + /* i686 caps */ + guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_I686, + NULL, NULL, 0, NULL); + if (!guest) + goto error; + + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HYPERV, NULL, NULL, 0, NULL)) + goto error; + + /* x86_64 caps */ + guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64, + NULL, NULL, 0, NULL); + if (!guest) + goto error; + + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HYPERV, NULL, NULL, 0, NULL)) + goto error; + + return caps; + + error: + virObjectUnref(caps); + return NULL; +} + + + /* * Driver functions */ @@ -267,6 +338,9 @@ hypervFreePrivate(hypervPrivate **priv) wsmc_release((*priv)->client); } + if ((*priv)->caps) + virObjectUnref((*priv)->caps); + hypervFreeParsedUri(&(*priv)->parsedUri); VIR_FREE(*priv); } @@ -377,6 +451,11 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, if (hypervInitConnection(conn, priv, username, password) < 0) goto cleanup; + /* set up capabilities */ + priv->caps = hypervCapsInit(priv); + if (!priv->caps) + goto cleanup; + conn->privateData = priv; priv = NULL; result = VIR_DRV_OPEN_SUCCESS; @@ -433,6 +512,16 @@ hypervConnectGetHostname(virConnectPtr conn) +static char* +hypervConnectGetCapabilities(virConnectPtr conn) +{ + hypervPrivate *priv = conn->privateData; + + return virCapabilitiesFormatXML(priv->caps); +} + + + static int hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info) { @@ -1602,6 +1691,7 @@ static virHypervisorDriver hypervHypervisorDriver = { .connectGetType = hypervConnectGetType, /* 0.9.5 */ .connectGetHostname = hypervConnectGetHostname, /* 0.9.5 */ .nodeGetInfo = hypervNodeGetInfo, /* 0.9.5 */ + .connectGetCapabilities = hypervConnectGetCapabilities, /* 6.9.0 */ .connectListDomains = hypervConnectListDomains, /* 0.9.5 */ .connectNumOfDomains = hypervConnectNumOfDomains, /* 0.9.5 */ .connectListAllDomains = hypervConnectListAllDomains, /* 0.10.2 */ diff --git a/src/hyperv/hyperv_private.h b/src/hyperv/hyperv_private.h index b502e71d83..cf08bf542b 100644 --- a/src/hyperv/hyperv_private.h +++ b/src/hyperv/hyperv_private.h @@ -26,6 +26,7 @@ #include "virerror.h" #include "hyperv_util.h" #include "openwsman.h" +#include "capabilities.h" typedef enum _hypervWmiVersion hypervWmiVersion; enum _hypervWmiVersion { @@ -38,4 +39,5 @@ struct _hypervPrivate { hypervParsedUri *parsedUri; WsManClient *client; hypervWmiVersion wmiVersion; + virCapsPtr caps; }; -- 2.27.0
From: Matt Coleman <matt@datto.com> Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- NEWS.rst | 6 +++--- src/hyperv/hyperv_driver.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/NEWS.rst b/NEWS.rst index 65fa616a03..c52dddde74 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -13,10 +13,10 @@ v6.9.0 (unreleased) * **New features** - * hyperv: implement ``virConnectGetCapabilities()`` + * hyperv: implement new APIs - The ``virConnectGetCapabilities()`` API has been implemented in the Hyper-V - driver. + The ``virConnectGetCapabilities()`` and ``virConnectGetMaxVcpus()`` APIs + have been implemented in the Hyper-V driver. * **Improvements** diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index b1f84c568f..faeb826601 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -522,6 +522,39 @@ hypervConnectGetCapabilities(virConnectPtr conn) +static int +hypervConnectGetMaxVcpus(virConnectPtr conn, const char *type G_GNUC_UNUSED) +{ + int result = -1; + hypervPrivate *priv = conn->privateData; + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + Msvm_ProcessorSettingData *processorSettingData = NULL; + + /* Get max processors definition */ + virBufferAddLit(&query, + MSVM_PROCESSORSETTINGDATA_WQL_SELECT + "WHERE InstanceID LIKE 'Microsoft:Definition%Maximum'"); + + if (hypervGetWmiClass(Msvm_ProcessorSettingData, &processorSettingData) < 0) + goto cleanup; + + if (!processorSettingData) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get maximum definition of Msvm_ProcessorSettingData for host %s"), + conn->uri->server); + goto cleanup; + } + + result = processorSettingData->data.common->VirtualQuantity; + + cleanup: + hypervFreeObject(priv, (hypervObject *) processorSettingData); + + return result; +} + + + static int hypervNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info) { @@ -1690,6 +1723,7 @@ static virHypervisorDriver hypervHypervisorDriver = { .connectClose = hypervConnectClose, /* 0.9.5 */ .connectGetType = hypervConnectGetType, /* 0.9.5 */ .connectGetHostname = hypervConnectGetHostname, /* 0.9.5 */ + .connectGetMaxVcpus = hypervConnectGetMaxVcpus, /* 6.9.0 */ .nodeGetInfo = hypervNodeGetInfo, /* 0.9.5 */ .connectGetCapabilities = hypervConnectGetCapabilities, /* 6.9.0 */ .connectListDomains = hypervConnectListDomains, /* 0.9.5 */ -- 2.27.0
From: Matt Coleman <matt@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. Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- NEWS.rst | 4 +- src/hyperv/hyperv_driver.c | 86 +++++++++++++++++++++++++++ src/hyperv/hyperv_wmi_generator.input | 2 +- 3 files changed, 89 insertions(+), 3 deletions(-) diff --git a/NEWS.rst b/NEWS.rst index c52dddde74..05e129d9cf 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -15,8 +15,8 @@ v6.9.0 (unreleased) * hyperv: implement new APIs - The ``virConnectGetCapabilities()`` and ``virConnectGetMaxVcpus()`` APIs - have been implemented in the Hyper-V driver. + The ``virConnectGetCapabilities()``, ``virConnectGetMaxVcpus()``, and + ``virConnectGetVersion()`` APIs have been implemented in the Hyper-V driver. * **Improvements** diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index faeb826601..14ebb4b62d 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -29,6 +29,7 @@ #include "viralloc.h" #include "virlog.h" #include "viruuid.h" +#include "virutil.h" #include "hyperv_driver.h" #include "hyperv_private.h" #include "hyperv_util.h" @@ -492,6 +493,90 @@ hypervConnectGetType(virConnectPtr conn G_GNUC_UNUSED) +static int +hypervConnectGetVersion(virConnectPtr conn, unsigned long *version) +{ + int result = -1; + hypervPrivate *priv = conn->privateData; + Win32_OperatingSystem *os = NULL; + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + char *suffix; + unsigned int major, minor, micro; + g_auto(virBuffer) mangled_version = VIR_BUFFER_INITIALIZER; + + virBufferAddLit(&query, WIN32_OPERATINGSYSTEM_WQL_SELECT); + + if (hypervGetWmiClass(Win32_OperatingSystem, &os) < 0) + goto cleanup; + + if (!os) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get version information for host %s"), + conn->uri->server); + goto cleanup; + } + + /* + * 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 + */ + 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; + } + + 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; + } + + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *) os); + + return result; +} + + + static char * hypervConnectGetHostname(virConnectPtr conn) { @@ -1722,6 +1807,7 @@ static virHypervisorDriver hypervHypervisorDriver = { .connectOpen = hypervConnectOpen, /* 0.9.5 */ .connectClose = hypervConnectClose, /* 0.9.5 */ .connectGetType = hypervConnectGetType, /* 0.9.5 */ + .connectGetVersion = hypervConnectGetVersion, /* 6.9.0 */ .connectGetHostname = hypervConnectGetHostname, /* 0.9.5 */ .connectGetMaxVcpus = hypervConnectGetMaxVcpus, /* 6.9.0 */ .nodeGetInfo = hypervNodeGetInfo, /* 0.9.5 */ 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 -- 2.27.0
On Thursday, 1 October 2020 23:47:16 CEST mcoleman@datto.com wrote:
From: Matt Coleman <matt@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, µ) < 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
On Oct 2, 2020, at 2:48 AM, Pino Toscano <ptoscano@redhat.com> wrote:
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.
You’re absolutely right. I’ll add some documentation about this. Would you prefer it as a separate commit or included with the code changes?
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.
I agree that it's extra work. I was being careful and wanted to ensure that all of the existing error handling in virParseVersionString() still occurred. After looking it over again, I was being excessively cautious and will change it to just produce the unsigned long. -- Matt
On Saturday, 3 October 2020 01:02:47 CEST Matt Coleman wrote:
On Oct 2, 2020, at 2:48 AM, Pino Toscano <ptoscano@redhat.com> wrote:
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.
You’re absolutely right. I’ll add some documentation about this. Would you prefer it as a separate commit or included with the code changes?
IMHO it makes sense in the same commit, as it would be the new logic and the documentation for it (in both code comments and rST for users) added at the same time. -- Pino Toscano
From: Matt Coleman <matt@datto.com> Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- NEWS.rst | 5 +++-- src/hyperv/hyperv_driver.c | 40 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/NEWS.rst b/NEWS.rst index 05e129d9cf..ed6a0b5176 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -15,8 +15,9 @@ v6.9.0 (unreleased) * hyperv: implement new APIs - The ``virConnectGetCapabilities()``, ``virConnectGetMaxVcpus()``, and - ``virConnectGetVersion()`` APIs have been implemented in the Hyper-V driver. + The ``virConnectGetCapabilities()``, ``virConnectGetMaxVcpus()``, + ``virConnectGetVersion()``, and ``virDomainGetAutostart()`` APIs have been + implemented in the Hyper-V driver. * **Improvements** diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 14ebb4b62d..9fbd944437 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1280,6 +1280,45 @@ hypervDomainCreate(virDomainPtr domain) +static int +hypervDomainGetAutostart(virDomainPtr domain, int *autostart) +{ + int result = -1; + char uuid_string[VIR_UUID_STRING_BUFLEN]; + hypervPrivate *priv = domain->conn->privateData; + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + Msvm_VirtualSystemGlobalSettingData *vsgsd = NULL; + Msvm_VirtualSystemSettingData *vssd = NULL; + + virUUIDFormat(domain->uuid, uuid_string); + + if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { + virBufferEscapeSQL(&query, + MSVM_VIRTUALSYSTEMGLOBALSETTINGDATA_WQL_SELECT + "WHERE SystemName = \"%s\"", uuid_string); + + if (hypervGetWmiClass(Msvm_VirtualSystemGlobalSettingData, &vsgsd) < 0) + goto cleanup; + + *autostart = vsgsd->data.common->AutomaticStartupAction == 2; + result = 0; + } else { + if (hypervGetVSSDFromUUID(priv, uuid_string, &vssd) < 0) + goto cleanup; + + *autostart = vssd->data.v2->AutomaticStartupAction == 4; + result = 0; + } + + cleanup: + hypervFreeObject(priv, (hypervObject *) vsgsd); + hypervFreeObject(priv, (hypervObject *) vssd); + + return result; +} + + + static int hypervConnectIsEncrypted(virConnectPtr conn) { @@ -1830,6 +1869,7 @@ static virHypervisorDriver hypervHypervisorDriver = { .connectNumOfDefinedDomains = hypervConnectNumOfDefinedDomains, /* 0.9.5 */ .domainCreate = hypervDomainCreate, /* 0.9.5 */ .domainCreateWithFlags = hypervDomainCreateWithFlags, /* 0.9.5 */ + .domainGetAutostart = hypervDomainGetAutostart, /* 6.9.0 */ .connectIsEncrypted = hypervConnectIsEncrypted, /* 0.9.5 */ .connectIsSecure = hypervConnectIsSecure, /* 0.9.5 */ .domainIsActive = hypervDomainIsActive, /* 0.9.5 */ -- 2.27.0
On Thu, Oct 1, 2020 at 5:51 PM <mcoleman@datto.com> wrote:
From: Matt Coleman <matt@datto.com>
These patches fix a couple bugs, consolidate duplicate code, and implement several APIs.
Currently, some interactions with Hyper-V systems fail when the system is not configured for the "en-US" locale. Additionally, some CPU names also contain the clock frequency, making it too long for _virNodeInfo.model. The first two patches fix these bugs.
The second two patches clean up the code a little: one moves repeated operations into new helper functions; the other replaces the generic "get WMI class list" functions with a macro.
The last four patches implement the following APIs in the Hyper-V driver: * virConnectGetCapabilities() * virConnectGetMaxVcpus() * virConnectGetVersion() * virDomainGetAutostart()
Matt Coleman (8): hyperv: make Msvm_ComputerSystem WQL queries locale agnostic hyperv: fix nodeGetInfo failures caused by long CPU names hyperv: break out common lookups into separate functions hyperv: replace generic WMI class list helpers with a macro hyperv: implement connectGetCapabilities hyperv: implement connectGetMaxVcpus hyperv: implement connectGetVersion hyperv: implement domainGetAutostart
NEWS.rst | 10 + src/hyperv/hyperv_driver.c | 691 ++++++++++++++++++-------- src/hyperv/hyperv_private.h | 2 + src/hyperv/hyperv_wmi.c | 87 +--- src/hyperv/hyperv_wmi.h | 34 +- src/hyperv/hyperv_wmi_classes.h | 4 +- src/hyperv/hyperv_wmi_generator.input | 2 +- 7 files changed, 524 insertions(+), 306 deletions(-)
-- 2.27.0
Are you sure your email configuration is set properly? It seems like there's a mismatch on your email addresses. I generally expect your authorship to be set to "Matt Coleman <mcoleman@datto.com>", but it seems like your latest email patch set is confused and sending emails as "<mcoleman@datto.com>" with a From line in the body as "Matt Coleman <matt@datto.com>". What's the correct email address for your authorship here? -- 真実はいつも一つ！/ Always, there's only one truth!
Are you sure your email configuration is set properly? It seems like there's a mismatch on your email addresses. I generally expect your authorship to be set to "Matt Coleman <mcoleman@datto.com>", but it seems like your latest email patch set is confused and sending emails as "<mcoleman@datto.com>" with a From line in the body as "Matt Coleman <matt@datto.com>".
What's the correct email address for your authorship here?
I’ve always done my git commits as “Matt Coleman <matt@datto.com>”, but it looks like that didn’t carry through with the mailing list-based workflow for my previous contributions. Some tool I used a little before 1am seems to have appended a `from` directive to the `sendemail` section of my `~/.gitconfig`, even though I wasn’t doing anything related to `git send-email` at that point. Luckily, I have that file in version control, so it was easy to confirm and revert. I very much look forward to libvirt.git switching to GitLab MRs. Some detail has gotten messed up in every single one of my submissions to the mailing list so far, and none of it would have happened in a GitLab MR. Matt
On Thursday, 1 October 2020 23:47:09 CEST mcoleman@datto.com wrote:
From: Matt Coleman <matt@datto.com>
These patches fix a couple bugs, consolidate duplicate code, and implement several APIs.
Nice! One general note for all the patches: NEWS.rst is updated as a whole at the end, as separate commit. This eases cherry-picking, removes dependencies between patches that would not conflict otherwise, and avoids churn in each patch. Thanks, -- Pino Toscano
On Oct 2, 2020, at 2:07 AM, Pino Toscano <ptoscano@redhat.com> wrote:
One general note for all the patches: NEWS.rst is updated as a whole at the end, as separate commit. This eases cherry-picking, removes dependencies between patches that would not conflict otherwise, and avoids churn in each patch.
Ironically, the last thing I did prior to submitting this to the mailing list was to split the NEWS.rst updates up and merge them with the commits that introduced the changes. :) Since the NEWS.rst update should be its own separate separate commit, I’ll be sending a v2 version of this whole patch set. -- Matt
participants (4)
-
Matt Coleman -
mcoleman＠datto.com
-
Neal Gompa -
Pino Toscano