This set of patches adds several new APIs and fixes several others. Changes since v1: * 'enabledValue' and 'disabledValue' are now static strings in hypervDomainSetAutostart() * a long virReportError() call has been split into two lines Matt Coleman (7): hyperv: implement domainSetAutostart hyperv: implement nodeGetFreeMemory hyperv: implement domainReboot and domainReset hyperv: implement domainShutdown and domainShutdownFlags hyperv: fix domainSuspend and domainResume on Hyper-V V2 hyperv: fix domainManagedSave on Hyper-V V2 news: more Hyper-V APIs NEWS.rst | 7 +- src/hyperv/hyperv_driver.c | 267 +++++++++++++++++++++++++- src/hyperv/hyperv_wmi_classes.h | 3 + src/hyperv/hyperv_wmi_generator.input | 78 ++++++++ 4 files changed, 347 insertions(+), 8 deletions(-) -- 2.27.0
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 91 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 2ac30fa4c6..79b48a9dff 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1310,6 +1310,96 @@ hypervDomainGetAutostart(virDomainPtr domain, int *autostart) +static int +hypervDomainSetAutostart(virDomainPtr domain, int autostart) +{ + int result = -1; + char uuid_string[VIR_UUID_STRING_BUFLEN]; + hypervPrivate *priv = domain->conn->privateData; + Msvm_VirtualSystemSettingData *vssd = NULL; + hypervInvokeParamsListPtr params = NULL; + g_auto(virBuffer) eprQuery = VIR_BUFFER_INITIALIZER; + virHashTablePtr autostartParam = NULL; + hypervWmiClassInfoListPtr embeddedParamClass = NULL; + const char *methodName = NULL, *embeddedParamName = NULL; + char enabledValue[] = "2", disabledValue[] = "0"; + + if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { + methodName = "ModifyVirtualSystem"; + embeddedParamName = "SystemSettingData"; + embeddedParamClass = Msvm_VirtualSystemGlobalSettingData_WmiInfo; + } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) { + methodName = "ModifySystemSettings"; + embeddedParamName = "SystemSettings"; + embeddedParamClass = Msvm_VirtualSystemSettingData_WmiInfo; + enabledValue[0] = '4'; + disabledValue[0] = '2'; + } + + virUUIDFormat(domain->uuid, uuid_string); + + if (hypervGetVSSDFromUUID(priv, uuid_string, &vssd) < 0) + goto cleanup; + + params = hypervCreateInvokeParamsList(priv, methodName, + MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR, + Msvm_VirtualSystemManagementService_WmiInfo); + + if (!params) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not create params")); + goto cleanup; + } + + if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { + virBufferEscapeSQL(&eprQuery, + MSVM_COMPUTERSYSTEM_WQL_SELECT "WHERE Name = '%s'", + uuid_string); + + if (hypervAddEprParam(params, "ComputerSystem", priv, &eprQuery, + Msvm_ComputerSystem_WmiInfo) < 0) + goto params_cleanup; + } + + autostartParam = hypervCreateEmbeddedParam(priv, embeddedParamClass); + + if (hypervSetEmbeddedProperty(autostartParam, "AutomaticStartupAction", + autostart ? enabledValue : disabledValue) < 0) { + hypervFreeEmbeddedParam(autostartParam); + goto params_cleanup; + } + + if (hypervSetEmbeddedProperty(autostartParam, "InstanceID", + vssd->data.common->InstanceID) < 0) { + hypervFreeEmbeddedParam(autostartParam); + goto params_cleanup; + } + + if (hypervAddEmbeddedParam(params, priv, embeddedParamName, autostartParam, + embeddedParamClass) < 0) { + hypervFreeEmbeddedParam(autostartParam); + goto params_cleanup; + } + + if (hypervInvokeMethod(priv, params, NULL) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not set autostart")); + goto cleanup; + } + + result = 0; + goto cleanup; + + params_cleanup: + hypervFreeInvokeParams(params); + cleanup: + hypervFreeObject(priv, (hypervObject *) vssd); + + return result; +} + + + static int hypervConnectIsEncrypted(virConnectPtr conn) { @@ -1861,6 +1951,7 @@ static virHypervisorDriver hypervHypervisorDriver = { .domainCreate = hypervDomainCreate, /* 0.9.5 */ .domainCreateWithFlags = hypervDomainCreateWithFlags, /* 0.9.5 */ .domainGetAutostart = hypervDomainGetAutostart, /* 6.9.0 */ + .domainSetAutostart = hypervDomainSetAutostart, /* 6.9.0 */ .connectIsEncrypted = hypervConnectIsEncrypted, /* 0.9.5 */ .connectIsSecure = hypervConnectIsSecure, /* 0.9.5 */ .domainIsActive = hypervDomainIsActive, /* 0.9.5 */ -- 2.27.0
On Tuesday, 13 October 2020 07:13:58 CEST Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 91 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 2ac30fa4c6..79b48a9dff 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1310,6 +1310,96 @@ hypervDomainGetAutostart(virDomainPtr domain, int *autostart)
+static int +hypervDomainSetAutostart(virDomainPtr domain, int autostart) +{ + int result = -1; + char uuid_string[VIR_UUID_STRING_BUFLEN]; + hypervPrivate *priv = domain->conn->privateData; + Msvm_VirtualSystemSettingData *vssd = NULL; + hypervInvokeParamsListPtr params = NULL; + g_auto(virBuffer) eprQuery = VIR_BUFFER_INITIALIZER; + virHashTablePtr autostartParam = NULL; + hypervWmiClassInfoListPtr embeddedParamClass = NULL; + const char *methodName = NULL, *embeddedParamName = NULL; + char enabledValue[] = "2", disabledValue[] = "0";
Not that it is an issue: IMHO using for enabledValue/disabledValue the same approach as methodName/embeddedParamName, i.e. simple const char* ponting to static strings, is easier. Also there is a bit of style mixup: - methodName/embeddedParamName/etc are NULL by default, set to a value for V1 or V2 (left NULL otherwise) - enabledValue/disabledValue are set to the values for V1, and changed to V2 - other patches in this series (#5 and #6) also do "V1 by default, change for V2" IMHO a single style would make things easier, especially in case there will ever be a V3.
+ if (hypervInvokeMethod(priv, params, NULL) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not set autostart"));
Minor question: is there really no way to get the reason (in form of string) of the failure? -- Pino Toscano
On Oct 14, 2020, at 3:24 AM, Pino Toscano <ptoscano@redhat.com> wrote:
On Tuesday, 13 October 2020 07:13:58 CEST Matt Coleman wrote:
+ const char *methodName = NULL, *embeddedParamName = NULL; + char enabledValue[] = "2", disabledValue[] = "0";
Not that it is an issue: IMHO using for enabledValue/disabledValue the same approach as methodName/embeddedParamName, i.e. simple const char* ponting to static strings, is easier.
I originally tried to declare them as const char*, but it failed to compile with the following error: ../src/hyperv/hyperv_driver.c: In function ‘hypervDomainSetAutostart’: ../src/hyperv/hyperv_driver.c:2707:56: error: passing argument 3 of ‘hypervSetEmbeddedProperty’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] 2707 | autostart ? enabledValue : disabledValue) < 0) { | ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~ In file included from ../src/hyperv/hyperv_driver.c:36: ../src/hyperv/hyperv_wmi.h:150:15: note: expected ‘char *’ but argument is of type ‘const char *’ 150 | char *value); | ~~~~~~^~~~~ hypervSetEmbeddedProperty() is just a wrapper for virHashUpdateEntry().
Also there is a bit of style mixup: - methodName/embeddedParamName/etc are NULL by default, set to a value for V1 or V2 (left NULL otherwise) - enabledValue/disabledValue are set to the values for V1, and changed to V2 - other patches in this series (#5 and #6) also do "V1 by default, change for V2" IMHO a single style would make things easier, especially in case there will ever be a V3.
I'll make these consistent. I think it'll be easier to maintain if the declarations set a NULL or dummy value and then configuration for all Hyper-V versions happens later in a conditional.
+ if (hypervInvokeMethod(priv, params, NULL) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not set autostart"));
Minor question: is there really no way to get the reason (in form of string) of the failure?
hypervInvokeMethod() only returns the XML response data for successful requests currently. I’ll look into changing it to always return the XML response data (if available). Thanks for the feedback on these patches. I will be submitting v3 of this patchset, but probably won’t get to it until early next week. -- Matt
On 10/13/20 7:13 AM, Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 91 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 2ac30fa4c6..79b48a9dff 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1310,6 +1310,96 @@ hypervDomainGetAutostart(virDomainPtr domain, int *autostart)
+static int +hypervDomainSetAutostart(virDomainPtr domain, int autostart) +{ + int result = -1; + char uuid_string[VIR_UUID_STRING_BUFLEN]; + hypervPrivate *priv = domain->conn->privateData; + Msvm_VirtualSystemSettingData *vssd = NULL; + hypervInvokeParamsListPtr params = NULL; + g_auto(virBuffer) eprQuery = VIR_BUFFER_INITIALIZER; + virHashTablePtr autostartParam = NULL; + hypervWmiClassInfoListPtr embeddedParamClass = NULL; + const char *methodName = NULL, *embeddedParamName = NULL; + char enabledValue[] = "2", disabledValue[] = "0"; + + if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { + methodName = "ModifyVirtualSystem"; + embeddedParamName = "SystemSettingData"; + embeddedParamClass = Msvm_VirtualSystemGlobalSettingData_WmiInfo; + } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) { + methodName = "ModifySystemSettings"; + embeddedParamName = "SystemSettings"; + embeddedParamClass = Msvm_VirtualSystemSettingData_WmiInfo; + enabledValue[0] = '4'; + disabledValue[0] = '2'; + }
As pino pointed out, this can be just one if.
+ + virUUIDFormat(domain->uuid, uuid_string); + + if (hypervGetVSSDFromUUID(priv, uuid_string, &vssd) < 0) + goto cleanup; + + params = hypervCreateInvokeParamsList(priv, methodName, + MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR, + Msvm_VirtualSystemManagementService_WmiInfo); + + if (!params) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not create params")); + goto cleanup; + } + + if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { + virBufferEscapeSQL(&eprQuery, + MSVM_COMPUTERSYSTEM_WQL_SELECT "WHERE Name = '%s'", + uuid_string); + + if (hypervAddEprParam(params, "ComputerSystem", priv, &eprQuery, + Msvm_ComputerSystem_WmiInfo) < 0) + goto params_cleanup; + } + + autostartParam = hypervCreateEmbeddedParam(priv, embeddedParamClass); + + if (hypervSetEmbeddedProperty(autostartParam, "AutomaticStartupAction", + autostart ? enabledValue : disabledValue) < 0) { + hypervFreeEmbeddedParam(autostartParam); + goto params_cleanup; + } + + if (hypervSetEmbeddedProperty(autostartParam, "InstanceID", + vssd->data.common->InstanceID) < 0) { + hypervFreeEmbeddedParam(autostartParam); + goto params_cleanup; + } + + if (hypervAddEmbeddedParam(params, priv, embeddedParamName, autostartParam, + embeddedParamClass) < 0) { + hypervFreeEmbeddedParam(autostartParam); + goto params_cleanup; + } + + if (hypervInvokeMethod(priv, params, NULL) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not set autostart")); + goto cleanup; + } + + result = 0; + goto cleanup; + + params_cleanup: + hypervFreeInvokeParams(params);
So the only reason for the separate lable is that hypervInvokeMethod() consumes @params and thus we must avoid jumping here once it was called. Fortunately, hypervFreeInvokeParams() accepts NULL so what we can do is to set params = NULL in both success and fail paths. I'll post a patch that makes hypervInvokeMethod() behave more sanely. Anyway, this is what I suggest is squashed in (I can squash it before pushing if you agree): diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 79b48a9dff..1eae7c38e2 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1309,7 +1309,6 @@ hypervDomainGetAutostart(virDomainPtr domain, int *autostart) } - static int hypervDomainSetAutostart(virDomainPtr domain, int autostart) { @@ -1320,15 +1319,12 @@ hypervDomainSetAutostart(virDomainPtr domain, int autostart) hypervInvokeParamsListPtr params = NULL; g_auto(virBuffer) eprQuery = VIR_BUFFER_INITIALIZER; virHashTablePtr autostartParam = NULL; - hypervWmiClassInfoListPtr embeddedParamClass = NULL; - const char *methodName = NULL, *embeddedParamName = NULL; + hypervWmiClassInfoListPtr embeddedParamClass = Msvm_VirtualSystemGlobalSettingData_WmiInfo; + const char *methodName = "ModifyVirtualSystem"; + const char *embeddedParamName = "SystemSettingData"; char enabledValue[] = "2", disabledValue[] = "0"; - if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { - methodName = "ModifyVirtualSystem"; - embeddedParamName = "SystemSettingData"; - embeddedParamClass = Msvm_VirtualSystemGlobalSettingData_WmiInfo; - } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) { + if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) { methodName = "ModifySystemSettings"; embeddedParamName = "SystemSettings"; embeddedParamClass = Msvm_VirtualSystemSettingData_WmiInfo; @@ -1342,8 +1338,8 @@ hypervDomainSetAutostart(virDomainPtr domain, int autostart) goto cleanup; params = hypervCreateInvokeParamsList(priv, methodName, - MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR, - Msvm_VirtualSystemManagementService_WmiInfo); + MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR, + Msvm_VirtualSystemManagementService_WmiInfo); if (!params) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1358,48 +1354,47 @@ hypervDomainSetAutostart(virDomainPtr domain, int autostart) if (hypervAddEprParam(params, "ComputerSystem", priv, &eprQuery, Msvm_ComputerSystem_WmiInfo) < 0) - goto params_cleanup; + goto cleanup; } autostartParam = hypervCreateEmbeddedParam(priv, embeddedParamClass); if (hypervSetEmbeddedProperty(autostartParam, "AutomaticStartupAction", - autostart ? enabledValue : disabledValue) < 0) { + autostart ? enabledValue : disabledValue) < 0) { hypervFreeEmbeddedParam(autostartParam); - goto params_cleanup; + goto cleanup; } if (hypervSetEmbeddedProperty(autostartParam, "InstanceID", vssd->data.common->InstanceID) < 0) { hypervFreeEmbeddedParam(autostartParam); - goto params_cleanup; + goto cleanup; } if (hypervAddEmbeddedParam(params, priv, embeddedParamName, autostartParam, embeddedParamClass) < 0) { hypervFreeEmbeddedParam(autostartParam); - goto params_cleanup; + goto cleanup; } if (hypervInvokeMethod(priv, params, NULL) < 0) { + params = NULL; virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not set autostart")); goto cleanup; } + params = NULL; result = 0; - goto cleanup; - params_cleanup: - hypervFreeInvokeParams(params); cleanup: + hypervFreeInvokeParams(params); hypervFreeObject(priv, (hypervObject *) vssd); return result; } - static int hypervConnectIsEncrypted(virConnectPtr conn) { Michal
On Oct 15, 2020, at 8:57 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 10/13/20 7:13 AM, Matt Coleman wrote:
+ char enabledValue[] = "2", disabledValue[] = "0"; + + if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { + methodName = "ModifyVirtualSystem"; + embeddedParamName = "SystemSettingData"; + embeddedParamClass = Msvm_VirtualSystemGlobalSettingData_WmiInfo; + } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) { + methodName = "ModifySystemSettings"; + embeddedParamName = "SystemSettings"; + embeddedParamClass = Msvm_VirtualSystemSettingData_WmiInfo; + enabledValue[0] = '4'; + disabledValue[0] = '2'; + }
As pino pointed out, this can be just one if.
Pino only suggested using a uniform approach but didn’t recommend a specific solution. Is the single if statement how you’d prefer to see it done? I think it would be clearer if it were more verbose: initially declare the variables either as NULL or with a dummy value, then set all of the variables’ values in conditional blocks for each supported Hyper-V version.
+ params_cleanup: + hypervFreeInvokeParams(params);
So the only reason for the separate lable is that hypervInvokeMethod() consumes @params and thus we must avoid jumping here once it was called. Fortunately, hypervFreeInvokeParams() accepts NULL so what we can do is to set params = NULL in both success and fail paths.
I'll post a patch that makes hypervInvokeMethod() behave more sanely.
Thanks for that patch - makes it much clearer.
Anyway, this is what I suggest is squashed in (I can squash it before pushing if you agree):
That sounds good to me unless you liked my earlier idea to be more verbose about the conditionals. If you do, then I think I should just revise the whole patchset. If you prefer the shorter conditionals, then it looks like we could go forward with 1, 2, 5, and 6 (if you can squash my requested change into it - see that thread) for now. There are some small changes I’d like to make to patches 3 and 4 in this series based on Pino’s comments. Based on the number of proposed squashed changes, the fact that I want to make changes to two commits in the middle of this set, and since slight changes will need to be made in order to work with your ownership transfer patchset, it seems to me like I should revise it all and send v3. Do you agree? Thanks for all the feedback! -- Matt
On 10/17/20 10:39 PM, Matt Coleman wrote:
On Oct 15, 2020, at 8:57 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 10/13/20 7:13 AM, Matt Coleman wrote:
+ char enabledValue[] = "2", disabledValue[] = "0"; + + if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) { + methodName = "ModifyVirtualSystem"; + embeddedParamName = "SystemSettingData"; + embeddedParamClass = Msvm_VirtualSystemGlobalSettingData_WmiInfo; + } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) { + methodName = "ModifySystemSettings"; + embeddedParamName = "SystemSettings"; + embeddedParamClass = Msvm_VirtualSystemSettingData_WmiInfo; + enabledValue[0] = '4'; + disabledValue[0] = '2'; + }
As pino pointed out, this can be just one if.
Pino only suggested using a uniform approach but didn’t recommend a specific solution. Is the single if statement how you’d prefer to see it done? I think it would be clearer if it were more verbose: initially declare the variables either as NULL or with a dummy value, then set all of the variables’ values in conditional blocks for each supported Hyper-V version.
I don't have a strong preference. But it seems a bit weird to init only some variables when declaring them to desired value and then have if() to init the rest. I understand why you needed to do it though => so that @enabledValue and @disabledValue are allocated and you could overwrite them in VERSION_V2 branch. Speaking of which, let me see if I can do something about that. As for you original question, how about initializing all variables to NULL and replacing if (version == _V1) { } else if (version == _V2) {} with a switch statement? My worry is that if we ever introduce _V3 these if-else statements won't catch it, with switch the compiler will notify us about missing case.
+ params_cleanup: + hypervFreeInvokeParams(params);
So the only reason for the separate lable is that hypervInvokeMethod() consumes @params and thus we must avoid jumping here once it was called. Fortunately, hypervFreeInvokeParams() accepts NULL so what we can do is to set params = NULL in both success and fail paths.
I'll post a patch that makes hypervInvokeMethod() behave more sanely.
Thanks for that patch - makes it much clearer.
Anyway, this is what I suggest is squashed in (I can squash it before pushing if you agree):
That sounds good to me unless you liked my earlier idea to be more verbose about the conditionals. If you do, then I think I should just revise the whole patchset.
If you prefer the shorter conditionals, then it looks like we could go forward with 1, 2, 5, and 6 (if you can squash my requested change into it - see that thread) for now. There are some small changes I’d like to make to patches 3 and 4 in this series based on Pino’s comments.
Based on the number of proposed squashed changes, the fact that I want to make changes to two commits in the middle of this set, and since slight changes will need to be made in order to work with your ownership transfer patchset, it seems to me like I should revise it all and send v3. Do you agree?
Right, at this point I agree that v3 is more clearer. Michal
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 79b48a9dff..c4fca4685e 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1400,6 +1400,33 @@ hypervDomainSetAutostart(virDomainPtr domain, int autostart) +static unsigned long long +hypervNodeGetFreeMemory(virConnectPtr conn) +{ + unsigned long long res = 0; + hypervPrivate *priv = conn->privateData; + Win32_OperatingSystem *operatingSystem = NULL; + g_auto(virBuffer) query = { g_string_new(WIN32_OPERATINGSYSTEM_WQL_SELECT), 0 }; + + if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0 || + !operatingSystem) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get free memory for host %s"), + conn->uri->server); + goto cleanup; + } + + /* Return free memory in bytes */ + res = operatingSystem->data.common->FreePhysicalMemory * 1024; + + cleanup: + hypervFreeObject(priv, (hypervObject *) operatingSystem); + + return res; +} + + + static int hypervConnectIsEncrypted(virConnectPtr conn) { @@ -1952,6 +1979,7 @@ static virHypervisorDriver hypervHypervisorDriver = { .domainCreateWithFlags = hypervDomainCreateWithFlags, /* 0.9.5 */ .domainGetAutostart = hypervDomainGetAutostart, /* 6.9.0 */ .domainSetAutostart = hypervDomainSetAutostart, /* 6.9.0 */ + .nodeGetFreeMemory = hypervNodeGetFreeMemory, /* 6.9.0 */ .connectIsEncrypted = hypervConnectIsEncrypted, /* 0.9.5 */ .connectIsSecure = hypervConnectIsSecure, /* 0.9.5 */ .domainIsActive = hypervDomainIsActive, /* 0.9.5 */ -- 2.27.0
On 10/13/20 7:13 AM, Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 79b48a9dff..c4fca4685e 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1400,6 +1400,33 @@ hypervDomainSetAutostart(virDomainPtr domain, int autostart)
+static unsigned long long +hypervNodeGetFreeMemory(virConnectPtr conn) +{ + unsigned long long res = 0; + hypervPrivate *priv = conn->privateData; + Win32_OperatingSystem *operatingSystem = NULL; + g_auto(virBuffer) query = { g_string_new(WIN32_OPERATINGSYSTEM_WQL_SELECT), 0 }; + + if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0 || + !operatingSystem) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get free memory for host %s"), + conn->uri->server);
IIUC, hypervGetWmiClass() reports an error (in fact more accurate one) on failure. So this overwrite doesn't look good. Also, there is no point calling free if @operatingSystem is NULL ;-) How about this squashed in? diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 0f6d3cb946..195cb4997a 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1403,20 +1403,19 @@ hypervNodeGetFreeMemory(virConnectPtr conn) Win32_OperatingSystem *operatingSystem = NULL; g_auto(virBuffer) query = { g_string_new(WIN32_OPERATINGSYSTEM_WQL_SELECT), 0 }; - if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0 || - !operatingSystem) { + if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0) + return 0; + + if (!operatingSystem) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not get free memory for host %s"), conn->uri->server); - goto cleanup; + return 0; } /* Return free memory in bytes */ res = operatingSystem->data.common->FreePhysicalMemory * 1024; - - cleanup: hypervFreeObject(priv, (hypervObject *) operatingSystem); - return res; } Michal
On Oct 15, 2020, at 8:56 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 10/13/20 7:13 AM, Matt Coleman wrote:
+ if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0 || + !operatingSystem) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get free memory for host %s"), + conn->uri->server);
IIUC, hypervGetWmiClass() reports an error (in fact more accurate one) on failure. So this overwrite doesn't look good. Also, there is no point calling free if @operatingSystem is NULL ;-)
Thanks for pointing all of that out.
How about this squashed in?
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 0f6d3cb946..195cb4997a 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1403,20 +1403,19 @@ hypervNodeGetFreeMemory(virConnectPtr conn) Win32_OperatingSystem *operatingSystem = NULL; g_auto(virBuffer) query = { g_string_new(WIN32_OPERATINGSYSTEM_WQL_SELECT), 0 };
- if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0 || - !operatingSystem) { + if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0) + return 0; + + if (!operatingSystem) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not get free memory for host %s"), conn->uri->server); - goto cleanup; + return 0; }
/* Return free memory in bytes */ res = operatingSystem->data.common->FreePhysicalMemory * 1024; - - cleanup: hypervFreeObject(priv, (hypervObject *) operatingSystem); - return res; }
This is a solid improvement. The pattern `if (hypervGetWmiClass() < 0 || !foo)` is used in several other places. Would you like me to fix those other occurrences? -- Matt
On 10/17/20 10:32 PM, Matt Coleman wrote:
On Oct 15, 2020, at 8:56 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 10/13/20 7:13 AM, Matt Coleman wrote:
+ if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0 || + !operatingSystem) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get free memory for host %s"), + conn->uri->server);
IIUC, hypervGetWmiClass() reports an error (in fact more accurate one) on failure. So this overwrite doesn't look good. Also, there is no point calling free if @operatingSystem is NULL ;-)
Thanks for pointing all of that out.
How about this squashed in?
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 0f6d3cb946..195cb4997a 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1403,20 +1403,19 @@ hypervNodeGetFreeMemory(virConnectPtr conn) Win32_OperatingSystem *operatingSystem = NULL; g_auto(virBuffer) query = { g_string_new(WIN32_OPERATINGSYSTEM_WQL_SELECT), 0 };
- if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0 || - !operatingSystem) { + if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0) + return 0; + + if (!operatingSystem) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not get free memory for host %s"), conn->uri->server); - goto cleanup; + return 0; }
/* Return free memory in bytes */ res = operatingSystem->data.common->FreePhysicalMemory * 1024; - - cleanup: hypervFreeObject(priv, (hypervObject *) operatingSystem); - return res; }
This is a solid improvement.
The pattern `if (hypervGetWmiClass() < 0 || !foo)` is used in several other places. Would you like me to fix those other occurrences?
Yes, please and thank you. I've opened hyperv driver code and realized it doesn't really match patterns used elsewhere. Any refactor that addresses that is more than welcome. Michal
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 48 +++++++++++++++++++++++++++++++++ src/hyperv/hyperv_wmi_classes.h | 1 + 2 files changed, 49 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index c4fca4685e..7182340f74 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -917,6 +917,52 @@ hypervDomainResume(virDomainPtr domain) +static int +hypervDomainReboot(virDomainPtr domain, unsigned int flags) +{ + int result = -1; + hypervPrivate *priv = domain->conn->privateData; + Msvm_ComputerSystem *computerSystem = NULL; + + virCheckFlags(0, -1); + + if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) + goto cleanup; + + result = hypervInvokeMsvmComputerSystemRequestStateChange(domain, + MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_REBOOT); + + cleanup: + hypervFreeObject(priv, (hypervObject *)computerSystem); + + return result; +} + + + +static int +hypervDomainReset(virDomainPtr domain, unsigned int flags) +{ + int result = -1; + hypervPrivate *priv = domain->conn->privateData; + Msvm_ComputerSystem *computerSystem = NULL; + + virCheckFlags(0, -1); + + if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) + goto cleanup; + + result = hypervInvokeMsvmComputerSystemRequestStateChange(domain, + MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_RESET); + + cleanup: + hypervFreeObject(priv, (hypervObject *)computerSystem); + + return result; +} + + + static int hypervDomainDestroyFlags(virDomainPtr domain, unsigned int flags) { @@ -1967,6 +2013,8 @@ static virHypervisorDriver hypervHypervisorDriver = { .domainLookupByName = hypervDomainLookupByName, /* 0.9.5 */ .domainSuspend = hypervDomainSuspend, /* 0.9.5 */ .domainResume = hypervDomainResume, /* 0.9.5 */ + .domainReboot = hypervDomainReboot, /* 6.9.0 */ + .domainReset = hypervDomainReset, /* 6.9.0 */ .domainDestroy = hypervDomainDestroy, /* 0.9.5 */ .domainDestroyFlags = hypervDomainDestroyFlags, /* 0.9.5 */ .domainGetOSType = hypervDomainGetOSType, /* 0.9.5 */ diff --git a/src/hyperv/hyperv_wmi_classes.h b/src/hyperv/hyperv_wmi_classes.h index d32711589a..7f4159dd8e 100644 --- a/src/hyperv/hyperv_wmi_classes.h +++ b/src/hyperv/hyperv_wmi_classes.h @@ -74,6 +74,7 @@ enum _Msvm_ComputerSystem_RequestedState { MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED = 2, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_DISABLED = 3, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_REBOOT = 10, + MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_RESET = 11, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_PAUSED = 32768, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_SUSPENDED = 32769, }; -- 2.27.0
On Tuesday, 13 October 2020 07:14:00 CEST Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 48 +++++++++++++++++++++++++++++++++ src/hyperv/hyperv_wmi_classes.h | 1 + 2 files changed, 49 insertions(+)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index c4fca4685e..7182340f74 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -917,6 +917,52 @@ hypervDomainResume(virDomainPtr domain)
+static int +hypervDomainReboot(virDomainPtr domain, unsigned int flags) +{ + int result = -1; + hypervPrivate *priv = domain->conn->privateData; + Msvm_ComputerSystem *computerSystem = NULL; + + virCheckFlags(0, -1); + + if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) + goto cleanup; + + result = hypervInvokeMsvmComputerSystemRequestStateChange(domain, + MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_REBOOT); + + cleanup: + hypervFreeObject(priv, (hypervObject *)computerSystem); + + return result; +} + + + +static int +hypervDomainReset(virDomainPtr domain, unsigned int flags) +{ + int result = -1; + hypervPrivate *priv = domain->conn->privateData; + Msvm_ComputerSystem *computerSystem = NULL; + + virCheckFlags(0, -1); + + if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) + goto cleanup; + + result = hypervInvokeMsvmComputerSystemRequestStateChange(domain, + MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_RESET); + + cleanup: + hypervFreeObject(priv, (hypervObject *)computerSystem); + + return result; +}
What about making a common helper function that calls hypervMsvmComputerSystemFromDomain + hypervInvokeMsvmComputerSystemRequestStateChange? Note that virDomainReboot() can take various flags, however none is used ATM. -- Pino Toscano
On Oct 13, 2020, at 3:40 AM, Pino Toscano <ptoscano@redhat.com> wrote:
What about making a common helper function that calls hypervMsvmComputerSystemFromDomain + hypervInvokeMsvmComputerSystemRequestStateChange?
I’ll include that in the next version of this patchset.
Note that virDomainReboot() can take various flags, however none is used ATM.
Would it be okay to stick with what I’ve got, now, and look at adding support for the flags in a future commit? -- Matt
On Wednesday, 14 October 2020 05:23:21 CEST Matt Coleman wrote:
Note that virDomainReboot() can take various flags, however none is used ATM.
Would it be okay to stick with what I’ve got, now, and look at adding support for the flags in a future commit?
Sorry, I did not express myself properly: I did not want to say you have to add support for these flags, but merely that the functions implemented by this patch do not. Hence, depending on what you plan to add to them (or even whether any of the flags can be supported), my earlier suggestion of a common helper might make sense or not. -- Pino Toscano
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 77 ++++++++++++++++++++++++++ src/hyperv/hyperv_wmi_generator.input | 78 +++++++++++++++++++++++++++ 2 files changed, 155 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 7182340f74..024ba7c6f4 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -917,6 +917,81 @@ hypervDomainResume(virDomainPtr domain) +static int +hypervDomainShutdownFlags(virDomainPtr domain, unsigned int flags) +{ + int result = -1; + hypervPrivate *priv = domain->conn->privateData; + Msvm_ComputerSystem *computerSystem = NULL; + Msvm_ShutdownComponent *shutdown = NULL; + bool in_transition = false; + char uuid[VIR_UUID_STRING_BUFLEN]; + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + hypervInvokeParamsListPtr params = NULL; + g_autofree char *selector = NULL; + + virCheckFlags(0, -1); + + virUUIDFormat(domain->uuid, uuid); + + if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) + goto cleanup; + + if (!hypervIsMsvmComputerSystemActive(computerSystem, &in_transition) || + in_transition) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Domain is not active or in state transition")); + goto cleanup; + } + + virBufferEscapeSQL(&query, + MSVM_SHUTDOWNCOMPONENT_WQL_SELECT + "WHERE SystemName = '%s'", uuid); + + if (hypervGetWmiClass(Msvm_ShutdownComponent, &shutdown) < 0 || + !shutdown) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Could not get Msvm_ShutdownComponent for domain with UUID '%s'"), + uuid); + goto cleanup; + } + + selector = g_strdup_printf( + "CreationClassName=\"Msvm_ShutdownComponent\"&DeviceID=\"%s\"&" + "SystemCreationClassName=\"Msvm_ComputerSystem\"&SystemName=\"%s\"", + shutdown->data.common->DeviceID, uuid); + + params = hypervCreateInvokeParamsList(priv, "InitiateShutdown", selector, + Msvm_ShutdownComponent_WmiInfo); + + hypervAddSimpleParam(params, "Force", "False"); + hypervAddSimpleParam(params, "Reason", _("Planned shutdown via libvirt")); + + if (hypervInvokeMethod(priv, params, NULL) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not shutdown domain with UUID '%s'"), uuid); + goto cleanup; + } + + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *) computerSystem); + hypervFreeObject(priv, (hypervObject *) shutdown); + + return result; +} + + + +static int +hypervDomainShutdown(virDomainPtr domain) +{ + return hypervDomainShutdownFlags(domain, 0); +} + + + static int hypervDomainReboot(virDomainPtr domain, unsigned int flags) { @@ -2013,6 +2088,8 @@ static virHypervisorDriver hypervHypervisorDriver = { .domainLookupByName = hypervDomainLookupByName, /* 0.9.5 */ .domainSuspend = hypervDomainSuspend, /* 0.9.5 */ .domainResume = hypervDomainResume, /* 0.9.5 */ + .domainShutdown = hypervDomainShutdown, /* 6.9.0 */ + .domainShutdownFlags = hypervDomainShutdownFlags, /* 6.9.0 */ .domainReboot = hypervDomainReboot, /* 6.9.0 */ .domainReset = hypervDomainReset, /* 6.9.0 */ .domainDestroy = hypervDomainDestroy, /* 0.9.5 */ diff --git a/src/hyperv/hyperv_wmi_generator.input b/src/hyperv/hyperv_wmi_generator.input index bbca550790..1377138a12 100644 --- a/src/hyperv/hyperv_wmi_generator.input +++ b/src/hyperv/hyperv_wmi_generator.input @@ -1072,3 +1072,81 @@ class v2/Msvm_Keyboard uint16 Password boolean UnicodeSupported end + + +class Msvm_ShutdownComponent + string Caption + string Description + string ElementName + datetime InstallDate + string Name + uint16 OperationalStatus[] + string StatusDescriptions[] + string Status + uint16 HealthState + uint16 EnabledState + string OtherEnabledState + uint16 RequestedState + uint16 EnabledDefault + datetime TimeOfLastStateChange + string SystemCreationClassName + string SystemName + string CreationClassName + string DeviceID + boolean PowerManagementSupported + uint16 PowerManagementCapabilities[] + uint16 Availability + uint16 StatusInfo + uint32 LastErrorCode + string ErrorDescription + boolean ErrorCleared + string OtherIdentifyingInfo[] + uint64 PowerOnHours + uint64 TotalPowerOnHours + string IdentifyingDescriptions[] + uint16 AdditionalAvailability[] + uint64 MaxQuiesceTime + uint16 LocationIndicator +end + + +class v2/Msvm_ShutdownComponent + string InstanceID + string Caption + string Description + string ElementName + datetime InstallDate + string Name + uint16 OperationalStatus[] + string StatusDescriptions[] + string Status + uint16 HealthState + uint16 CommunicationStatus + uint16 DetailedStatus + uint16 OperatingStatus + uint16 PrimaryStatus + uint16 EnabledState + string OtherEnabledState + uint16 RequestedState + uint16 EnabledDefault + datetime TimeOfLastStateChange + uint16 AvailableRequestedStates[] + uint16 TransitioningToState + string SystemCreationClassName + string SystemName + string CreationClassName + string DeviceID + boolean PowerManagementSupported + uint16 PowerManagementCapabilities[] + uint16 Availability + uint16 StatusInfo + uint32 LastErrorCode + string ErrorDescription + boolean ErrorCleared + string OtherIdentifyingInfo[] + uint64 PowerOnHours + uint64 TotalPowerOnHours + string IdentifyingDescriptions[] + uint16 AdditionalAvailability[] + uint64 MaxQuiesceTime +end -- 2.27.0
On Tuesday, 13 October 2020 07:14:01 CEST Matt Coleman wrote:
+ hypervAddSimpleParam(params, "Force", "False"); + hypervAddSimpleParam(params, "Reason", _("Planned shutdown via libvirt"));
Is this message logged in the Hyper-V logs? If so, then I'd not translate it: imagine an user running libvirt on an e.g. a French locale, sending this query to an Hyper-V on e.g. German. While English is not that different in this situation (still potentially not the Hyper-V language), at least IMHO it gives something understandable. Of course, in case the message is changed to an untranslated text, then please leave a comment there, so noone accidentally makes it translatable. -- Pino Toscano
On Oct 13, 2020, at 3:55 AM, Pino Toscano <ptoscano@redhat.com> wrote:
On Tuesday, 13 October 2020 07:14:01 CEST Matt Coleman wrote:
+ hypervAddSimpleParam(params, "Force", "False"); + hypervAddSimpleParam(params, "Reason", _("Planned shutdown via libvirt"));
Is this message logged in the Hyper-V logs? If so, then I'd not translate it: imagine an user running libvirt on an e.g. a French locale, sending this query to an Hyper-V on e.g. German. While English is not that different in this situation (still potentially not the Hyper-V language), at least IMHO it gives something understandable.
Of course, in case the message is changed to an untranslated text, then please leave a comment there, so noone accidentally makes it translatable.
This text does get logged. I’ll remove the translation and add a comment. -- Matt
Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 15 +++++++++++---- src/hyperv/hyperv_wmi_classes.h | 1 + 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 024ba7c6f4..aeee5b2b9f 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -867,6 +867,10 @@ hypervDomainSuspend(virDomainPtr domain) int result = -1; hypervPrivate *priv = domain->conn->privateData; Msvm_ComputerSystem *computerSystem = NULL; + int requestedState = MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_PAUSED; + + if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) + requestedState = MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_QUIESCE; if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) goto cleanup; @@ -878,8 +882,8 @@ hypervDomainSuspend(virDomainPtr domain) goto cleanup; } - result = hypervInvokeMsvmComputerSystemRequestStateChange - (domain, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_PAUSED); + result = hypervInvokeMsvmComputerSystemRequestStateChange(domain, + requestedState); cleanup: hypervFreeObject(priv, (hypervObject *)computerSystem); @@ -895,12 +899,15 @@ hypervDomainResume(virDomainPtr domain) int result = -1; hypervPrivate *priv = domain->conn->privateData; Msvm_ComputerSystem *computerSystem = NULL; + int expectedState = MSVM_COMPUTERSYSTEM_ENABLEDSTATE_PAUSED; + + if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) + expectedState = MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_QUIESCE; if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) goto cleanup; - if (computerSystem->data.common->EnabledState != - MSVM_COMPUTERSYSTEM_ENABLEDSTATE_PAUSED) { + if (computerSystem->data.common->EnabledState != expectedState) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not paused")); goto cleanup; diff --git a/src/hyperv/hyperv_wmi_classes.h b/src/hyperv/hyperv_wmi_classes.h index 7f4159dd8e..0074d8889e 100644 --- a/src/hyperv/hyperv_wmi_classes.h +++ b/src/hyperv/hyperv_wmi_classes.h @@ -73,6 +73,7 @@ enum _Msvm_ComputerSystem_EnabledState { enum _Msvm_ComputerSystem_RequestedState { MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED = 2, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_DISABLED = 3, + MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_QUIESCE = 9, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_REBOOT = 10, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_RESET = 11, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_PAUSED = 32768, -- 2.27.0
On 10/13/20 7:14 AM, Matt Coleman wrote:
Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 15 +++++++++++---- src/hyperv/hyperv_wmi_classes.h | 1 + 2 files changed, 12 insertions(+), 4 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 8 ++++++-- src/hyperv/hyperv_wmi_classes.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index aeee5b2b9f..fb46f42631 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1644,6 +1644,10 @@ hypervDomainManagedSave(virDomainPtr domain, unsigned int flags) hypervPrivate *priv = domain->conn->privateData; Msvm_ComputerSystem *computerSystem = NULL; bool in_transition = false; + int requestedState = MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_SUSPENDED; + + if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) + requestedState = MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED_BUT_OFFLINE; virCheckFlags(0, -1); @@ -1657,8 +1661,8 @@ hypervDomainManagedSave(virDomainPtr domain, unsigned int flags) goto cleanup; } - result = hypervInvokeMsvmComputerSystemRequestStateChange - (domain, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_SUSPENDED); + result = hypervInvokeMsvmComputerSystemRequestStateChange(domain, + requestedState); cleanup: hypervFreeObject(priv, (hypervObject *)computerSystem); diff --git a/src/hyperv/hyperv_wmi_classes.h b/src/hyperv/hyperv_wmi_classes.h index 0074d8889e..a5213901c8 100644 --- a/src/hyperv/hyperv_wmi_classes.h +++ b/src/hyperv/hyperv_wmi_classes.h @@ -73,6 +73,7 @@ enum _Msvm_ComputerSystem_EnabledState { enum _Msvm_ComputerSystem_RequestedState { MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED = 2, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_DISABLED = 3, + MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED_BUT_OFFLINE = 6, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_QUIESCE = 9, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_REBOOT = 10, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_RESET = 11, -- 2.27.0
On Oct 13, 2020, at 1:14 AM, Matt Coleman <mcoleman@datto.com> wrote:
+ if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) + requestedState = MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED_BUT_OFFLINE; ... + MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED_BUT_OFFLINE = 6,
I realized I named this after the description of the (related) Msvm_ComputerSystem EnabledState value, not the RequestStateChange() method’s RequestedState parameter that this enum actually represents. https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/msvm-computersystem... https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/requeststatechange-... If another squashed change is ok, this should be “MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_OFFLINE” instead of “MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED_BUT_OFFLINE”. I feel like we’ve discussed a lot of changes getting squashed into this patchset and that I should probably just revise it all and send v3. -- Matt
Signed-off-by: Matt Coleman <matt@datto.com> --- NEWS.rst | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/NEWS.rst b/NEWS.rst index bc35458f38..d0454b7840 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -22,8 +22,11 @@ v6.9.0 (unreleased) * hyperv: implement new APIs The ``virConnectGetCapabilities()``, ``virConnectGetMaxVcpus()``, - ``virConnectGetVersion()``, and ``virDomainGetAutostart()`` APIs have been - implemented in the Hyper-V driver. + ``virConnectGetVersion()``, ``virDomainGetAutostart()``, + ``virDomainSetAutostart()``, ``virNodeGetFreeMemory()``, + ``virDomainReboot()``, ``virDomainReset()``, ``virDomainShutdown()``, and + ``virDomainShutdownFlags()`` APIs have been implemented in the Hyper-V + driver. * bhyve: implement virtio-9p filesystem support -- 2.27.0
On 10/13/20 7:14 AM, Matt Coleman wrote:
Signed-off-by: Matt Coleman <matt@datto.com> --- NEWS.rst | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/NEWS.rst b/NEWS.rst index bc35458f38..d0454b7840 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -22,8 +22,11 @@ v6.9.0 (unreleased) * hyperv: implement new APIs
The ``virConnectGetCapabilities()``, ``virConnectGetMaxVcpus()``, - ``virConnectGetVersion()``, and ``virDomainGetAutostart()`` APIs have been - implemented in the Hyper-V driver. + ``virConnectGetVersion()``, ``virDomainGetAutostart()``, + ``virDomainSetAutostart()``, ``virNodeGetFreeMemory()``, + ``virDomainReboot()``, ``virDomainReset()``, ``virDomainShutdown()``, and + ``virDomainShutdownFlags()`` APIs have been implemented in the Hyper-V + driver.
* bhyve: implement virtio-9p filesystem support
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> I'll update the list if I get your blessing on 1/7 and 2/7 though. Michal
On Oct 13, 2020, at 1:13 AM, Matt Coleman <mcoleman@datto.com> wrote:
This set of patches adds several new APIs and fixes several others.
Changes since v1: * 'enabledValue' and 'disabledValue' are now static strings in hypervDomainSetAutostart() * a long virReportError() call has been split into two lines
I will be making some changes based on Pino’s review, so please don’t merge this version. I probably won’t be able to get to it until early next week. Thanks! -- Matt
participants (4)
-
Matt Coleman -
Michal Privoznik -
Michal Prívozník
-
Pino Toscano