12:13 a.m.
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
12:13 a.m.
New subject: [PATCH v2 1/7] hyperv: implement domainSetAutostart
Co-authored-by: Sri Ramanujam <sramanujam(a)datto.com>
Signed-off-by: Matt Coleman <matt(a)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
2:24 a.m.
New subject: [PATCH v2 1/7] hyperv: implement domainSetAutostart
On Tuesday, 13 October 2020 07:13:58 CEST Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam(a)datto.com>
Signed-off-by: Matt Coleman <matt(a)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
6:45 a.m.
New subject: [PATCH v2 1/7] hyperv: implement domainSetAutostart
On Oct 14, 2020, at 3:24 AM, Pino Toscano <ptoscano(a)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
7:57 a.m.
New subject: [PATCH v2 1/7] hyperv: implement domainSetAutostart
On 10/13/20 7:13 AM, Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam(a)datto.com>
Signed-off-by: Matt Coleman <matt(a)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
3:39 p.m.
New subject: [PATCH v2 1/7] hyperv: implement domainSetAutostart
On Oct 15, 2020, at 8:57 AM, Michal Privoznik
<mprivozn(a)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
4:33 a.m.
New subject: [PATCH v2 1/7] hyperv: implement domainSetAutostart
On 10/17/20 10:39 PM, Matt Coleman wrote:
> On Oct 15, 2020, at 8:57 AM, Michal Privoznik
<mprivozn(a)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
12:13 a.m.
New subject: [PATCH v2 2/7] hyperv: implement nodeGetFreeMemory
Co-authored-by: Sri Ramanujam <sramanujam(a)datto.com>
Signed-off-by: Matt Coleman <matt(a)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
7:56 a.m.
New subject: [PATCH v2 2/7] hyperv: implement nodeGetFreeMemory
On 10/13/20 7:13 AM, Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam(a)datto.com>
Signed-off-by: Matt Coleman <matt(a)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
3:32 p.m.
New subject: [PATCH v2 2/7] hyperv: implement nodeGetFreeMemory
On Oct 15, 2020, at 8:56 AM, Michal Privoznik
<mprivozn(a)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
4:18 a.m.
New subject: [PATCH v2 2/7] hyperv: implement nodeGetFreeMemory
On 10/17/20 10:32 PM, Matt Coleman wrote:
> On Oct 15, 2020, at 8:56 AM, Michal Privoznik
<mprivozn(a)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
12:14 a.m.
New subject: [PATCH v2 3/7] hyperv: implement domainReboot and domainReset
Co-authored-by: Sri Ramanujam <sramanujam(a)datto.com>
Signed-off-by: Matt Coleman <matt(a)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
2:40 a.m.
New subject: [PATCH v2 3/7] hyperv: implement domainReboot and domainReset
On Tuesday, 13 October 2020 07:14:00 CEST Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam(a)datto.com>
Signed-off-by: Matt Coleman <matt(a)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
10:23 p.m.
New subject: [PATCH v2 3/7] hyperv: implement domainReboot and domainReset
On Oct 13, 2020, at 3:40 AM, Pino Toscano <ptoscano(a)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
2:16 a.m.
New subject: [PATCH v2 3/7] hyperv: implement domainReboot and domainReset
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
12:14 a.m.
New subject: [PATCH v2 4/7] hyperv: implement domainShutdown and domainShutdownFlags
Co-authored-by: Sri Ramanujam <sramanujam(a)datto.com>
Signed-off-by: Matt Coleman <matt(a)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
2:55 a.m.
New subject: [PATCH v2 4/7] hyperv: implement domainShutdown and domainShutdownFlags
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
10:26 p.m.
New subject: [PATCH v2 4/7] hyperv: implement domainShutdown and domainShutdownFlags
On Oct 13, 2020, at 3:55 AM, Pino Toscano <ptoscano(a)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
12:14 a.m.
New subject: [PATCH v2 5/7] hyperv: fix domainSuspend and domainResume on Hyper-V V2
Signed-off-by: Matt Coleman <matt(a)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
7:56 a.m.
New subject: [PATCH v2 5/7] hyperv: fix domainSuspend and domainResume on Hyper-V V2
On 10/13/20 7:14 AM, Matt Coleman wrote:
Signed-off-by: Matt Coleman <matt(a)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(a)redhat.com>
Michal
12:14 a.m.
New subject: [PATCH v2 6/7] hyperv: fix domainManagedSave on Hyper-V V2
Signed-off-by: Matt Coleman <matt(a)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
3:32 p.m.
New subject: [PATCH v2 6/7] hyperv: fix domainManagedSave on Hyper-V V2
On Oct 13, 2020, at 1:14 AM, Matt Coleman <mcoleman(a)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-computersys...
https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/requeststatechan...
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
12:14 a.m.
New subject: [PATCH v2 7/7] news: more Hyper-V APIs
Signed-off-by: Matt Coleman <matt(a)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
7:56 a.m.
New subject: [PATCH v2 7/7] news: more Hyper-V APIs
On 10/13/20 7:14 AM, Matt Coleman wrote:
Signed-off-by: Matt Coleman <matt(a)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(a)redhat.com>
I'll update the list if I get your blessing on 1/7 and 2/7 though.
Michal
6:53 a.m.
On Oct 13, 2020, at 1:13 AM, Matt Coleman <mcoleman(a)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
1587
days inactive
1593
days old
24 comments
4 participants
participants (4)
-
Matt Coleman -
Michal Privoznik -
Michal Prívozník
-
Pino Toscano