[PATCH 00/11] hyperv: implement defining/undefining domains and
This series starts by fixing a some dangerous behavior due to ambiguous VM names, since Hyper-V allows multiple VMs to be defined with the same name. That meant that `virsh dumpxml` could return XML for the wrong domain. Additionally, when I implemented `undefine`, it would just select one of the domains with the given name. The majority of the series implements defining and undefining domains, as well as attaching storage devices to domains. Networking functionality will be in the next patch series. Here's a GitLab MR, if you'd prefer to review the changes there: https://gitlab.com/iammattcoleman/libvirt/-/merge_requests/14 Matt Coleman (11): hyperv: ambiguous VM names will throw an error hyperv: implement domainUndefine and domainUndefineFlags hyperv: implement domainCreateXML and domainDefineXML hyperv: add hypervMsvmVSMSAddResourceSettings hyperv: create SCSI controllers when defining domains hyperv: attach virtual disks when defining domains hyperv: attach physical disks when defining domains hyperv: attach virtual optical disks when defining domains hyperv: attach floppy disks when defining domains hyperv: implement domainAttachDevice and domainAttachDeviceFlags news: defining/undefining domains & device attachment for Hyper-V NEWS.rst | 7 +- include/libvirt/virterror.h | 1 + src/hyperv/hyperv_driver.c | 881 ++++++++++++++++++++++++++++++++ src/hyperv/hyperv_wmi.c | 52 ++ src/hyperv/hyperv_wmi.h | 5 + src/hyperv/hyperv_wmi_classes.h | 1 + src/util/virerror.c | 6 +- 7 files changed, 949 insertions(+), 4 deletions(-) -- 2.27.0
Since Hyper-V allows multiple VMs to be created with the same name, some commands produce unpredictable results due to hypervDomainLookupByName's WMI query selecting the wrong domain. For example, this prevents `virsh dumpxml` from outputting XML for the wrong domain. Signed-off-by: Matt Coleman <matt@datto.com> --- include/libvirt/virterror.h | 1 + src/hyperv/hyperv_driver.c | 7 +++++++ src/util/virerror.c | 6 ++++-- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index b96fe250aa..524a7bf9e8 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -333,6 +333,7 @@ typedef enum { VIR_ERR_NO_NETWORK_PORT = 107, /* network port not found */ VIR_ERR_NO_HOSTNAME = 108, /* no domain's hostname found */ VIR_ERR_CHECKPOINT_INCONSISTENT = 109, /* checkpoint can't be used */ + VIR_ERR_MULTIPLE_DOMAINS = 110, /* more than one matching domain found */ # ifdef VIR_ENUM_SENTINELS VIR_ERR_NUMBER_LAST diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index c2c103aa3b..6d81deb4d9 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1121,6 +1121,13 @@ hypervDomainLookupByName(virConnectPtr conn, const char *name) if (hypervGetVirtualSystemByName(priv, name, &computerSystem) < 0) goto cleanup; + if (computerSystem->next) { + virReportError(VIR_ERR_MULTIPLE_DOMAINS, + _("Multiple domains exist with the name '%s': repeat the request using a UUID"), + name); + goto cleanup; + } + hypervMsvmComputerSystemToDomain(conn, computerSystem, &domain); cleanup: diff --git a/src/util/virerror.c b/src/util/virerror.c index 9e3bad97eb..14054add41 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1229,8 +1229,10 @@ static const virErrorMsgTuple virErrorMsgStrings[] = { N_("no hostname found: %s") }, [VIR_ERR_CHECKPOINT_INCONSISTENT] = { N_("checkpoint inconsistent"), - N_("checkpoint inconsistent: %s") - }, + N_("checkpoint inconsistent: %s") }, + [VIR_ERR_MULTIPLE_DOMAINS] = { + N_("multiple matching domains found"), + N_("multiple matching domains found: %s") }, }; G_STATIC_ASSERT(G_N_ELEMENTS(virErrorMsgStrings) == VIR_ERR_NUMBER_LAST); -- 2.27.0
On Tue, Nov 24, 2020 at 02:48:30PM -0500, Matt Coleman wrote:
Since Hyper-V allows multiple VMs to be created with the same name, some commands produce unpredictable results due to hypervDomainLookupByName's WMI query selecting the wrong domain.
For example, this prevents `virsh dumpxml` from outputting XML for the wrong domain.
Signed-off-by: Matt Coleman <matt@datto.com> --- include/libvirt/virterror.h | 1 + src/hyperv/hyperv_driver.c | 7 +++++++ src/util/virerror.c | 6 ++++-- 3 files changed, 12 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 60 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 6d81deb4d9..8e16ff529f 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1874,6 +1874,64 @@ hypervDomainCreate(virDomainPtr domain) } +static int +hypervDomainUndefineFlags(virDomainPtr domain, unsigned int flags) +{ + int result = -1; + char uuid_string[VIR_UUID_STRING_BUFLEN]; + hypervPrivate *priv = domain->conn->privateData; + Msvm_ComputerSystem *computerSystem = NULL; + g_autoptr(hypervInvokeParamsList) params = NULL; + g_auto(virBuffer) eprQuery = VIR_BUFFER_INITIALIZER; + + virCheckFlags(0, -1); + + virUUIDFormat(domain->uuid, uuid_string); + + if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) + goto cleanup; + + /* try to shut down the VM if it's not disabled, just to be safe */ + if (computerSystem->data->EnabledState != MSVM_COMPUTERSYSTEM_ENABLEDSTATE_DISABLED && + hypervDomainShutdown(domain) < 0) { + goto cleanup; + } + + /* prepare params */ + params = hypervCreateInvokeParamsList("DestroySystem", + MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR, + Msvm_VirtualSystemManagementService_WmiInfo); + + if (!params) + goto cleanup; + + virBufferEscapeSQL(&eprQuery, + MSVM_COMPUTERSYSTEM_WQL_SELECT "WHERE Name = '%s'", + uuid_string); + + if (hypervAddEprParam(params, "AffectedSystem", &eprQuery, Msvm_ComputerSystem_WmiInfo) < 0) + goto cleanup; + + /* actually destroy the VM */ + if (hypervInvokeMethod(priv, ¶ms, NULL) < 0) + goto cleanup; + + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *)computerSystem); + + return result; +} + + +static int +hypervDomainUndefine(virDomainPtr domain) +{ + return hypervDomainUndefineFlags(domain, 0); +} + + static int hypervDomainGetAutostart(virDomainPtr domain, int *autostart) { @@ -2485,6 +2543,8 @@ static virHypervisorDriver hypervHypervisorDriver = { .connectNumOfDefinedDomains = hypervConnectNumOfDefinedDomains, /* 0.9.5 */ .domainCreate = hypervDomainCreate, /* 0.9.5 */ .domainCreateWithFlags = hypervDomainCreateWithFlags, /* 0.9.5 */ + .domainUndefine = hypervDomainUndefine, /* 6.10.0 */ + .domainUndefineFlags = hypervDomainUndefineFlags, /* 6.10.0 */ .domainGetAutostart = hypervDomainGetAutostart, /* 6.9.0 */ .domainSetAutostart = hypervDomainSetAutostart, /* 6.9.0 */ .domainGetSchedulerType = hypervDomainGetSchedulerType, /* 6.10.0 */ -- 2.27.0
On Tue, Nov 24, 2020 at 02:48:31PM -0500, Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 60 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 6d81deb4d9..8e16ff529f 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -1874,6 +1874,64 @@ hypervDomainCreate(virDomainPtr domain) }
+static int +hypervDomainUndefineFlags(virDomainPtr domain, unsigned int flags) +{ + int result = -1; + char uuid_string[VIR_UUID_STRING_BUFLEN]; + hypervPrivate *priv = domain->conn->privateData; + Msvm_ComputerSystem *computerSystem = NULL; + g_autoptr(hypervInvokeParamsList) params = NULL; + g_auto(virBuffer) eprQuery = VIR_BUFFER_INITIALIZER; + + virCheckFlags(0, -1); + + virUUIDFormat(domain->uuid, uuid_string); + + if (hypervMsvmComputerSystemFromDomain(domain, &computerSystem) < 0) + goto cleanup; + + /* try to shut down the VM if it's not disabled, just to be safe */ + if (computerSystem->data->EnabledState != MSVM_COMPUTERSYSTEM_ENABLEDSTATE_DISABLED && + hypervDomainShutdown(domain) < 0) { + goto cleanup; + }
Many, but not all, drivers in libvirt allow undefining the cofig for a running VM. ie we can delete the config on disk, without affecting the running VM. This results in a so called "transient" VM - basically a VM which only exists as long as it is running - virDomainCreateXML creates such a beast, while virDomainDefineXML+virDomainCreate creates a "persistent" VM and starts it. If hyperv doesn't allow that, then shutting down the VM is likely to be surprising. I'd suggest we report an error indicating undefine is not permitted for a running VM in such a case.
+ + /* prepare params */ + params = hypervCreateInvokeParamsList("DestroySystem", + MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR, + Msvm_VirtualSystemManagementService_WmiInfo); + + if (!params) + goto cleanup; + + virBufferEscapeSQL(&eprQuery, + MSVM_COMPUTERSYSTEM_WQL_SELECT "WHERE Name = '%s'", + uuid_string); + + if (hypervAddEprParam(params, "AffectedSystem", &eprQuery, Msvm_ComputerSystem_WmiInfo) < 0) + goto cleanup; + + /* actually destroy the VM */ + if (hypervInvokeMethod(priv, ¶ms, NULL) < 0) + goto cleanup; + + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *)computerSystem); + + return result; +} + + +static int +hypervDomainUndefine(virDomainPtr domain) +{ + return hypervDomainUndefineFlags(domain, 0); +} + + static int hypervDomainGetAutostart(virDomainPtr domain, int *autostart) { @@ -2485,6 +2543,8 @@ static virHypervisorDriver hypervHypervisorDriver = { .connectNumOfDefinedDomains = hypervConnectNumOfDefinedDomains, /* 0.9.5 */ .domainCreate = hypervDomainCreate, /* 0.9.5 */ .domainCreateWithFlags = hypervDomainCreateWithFlags, /* 0.9.5 */ + .domainUndefine = hypervDomainUndefine, /* 6.10.0 */ + .domainUndefineFlags = hypervDomainUndefineFlags, /* 6.10.0 */ .domainGetAutostart = hypervDomainGetAutostart, /* 6.9.0 */ .domainSetAutostart = hypervDomainSetAutostart, /* 6.9.0 */ .domainGetSchedulerType = hypervDomainGetSchedulerType, /* 6.10.0 */ -- 2.27.0
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Nov 26, 2020, at 9:26 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Nov 24, 2020 at 02:48:31PM -0500, Matt Coleman wrote:
+ /* try to shut down the VM if it's not disabled, just to be safe */ + if (computerSystem->data->EnabledState != MSVM_COMPUTERSYSTEM_ENABLEDSTATE_DISABLED && + hypervDomainShutdown(domain) < 0) { + goto cleanup; + }
Many, but not all, drivers in libvirt allow undefining the cofig for a running VM. ie we can delete the config on disk, without affecting the running VM. This results in a so called "transient" VM - basically a VM which only exists as long as it is running - virDomainCreateXML creates such a beast, while virDomainDefineXML+virDomainCreate creates a "persistent" VM and starts it.
If hyperv doesn't allow that, then shutting down the VM is likely to be surprising.
I'd suggest we report an error indicating undefine is not permitted for a running VM in such a case.
Hyper-V does not allow a VM's definition to be deleted if it's active, paused, or in state transition. If I remove the check entirely, it displays an error including Hyper-V's method name, the error code from Hyper-V, and a string that's generated by hypervReturnCodeToString() from hyperv_wmi.c. For example, `virsh undefine runningvm` would throw the error: error: Failed to undefine domain runningvm error: internal error: Invocation of DestroySystem returned an error: Invalid state for this operation (32775) That simplifies the code significantly and provides details (the method name and error code) that users could look up in Microsoft's documentation. However, I don't think it's very clearly phrased. "Domain must not be active or in state transition" with the error code set to VIR_ERR_OPERATION_INVALID produces the following error output: error: Failed to undefine domain testvm error: Requested operation is not valid: Domain must not be active or in state transition It's a difference of 18 lines (51 vs 33). The shorter version eliminates the result and computerSystem variables, does not use goto, and only sends a single WMI request to Hyper-V since it does not have to query the VM state before attempting to undefine it. Would you prefer to have simpler code or a custom error message? Thanks! Matt
On Thu, Jan 07, 2021 at 06:55:45PM -0500, Matt Coleman wrote:
On Nov 26, 2020, at 9:26 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Nov 24, 2020 at 02:48:31PM -0500, Matt Coleman wrote:
+ /* try to shut down the VM if it's not disabled, just to be safe */ + if (computerSystem->data->EnabledState != MSVM_COMPUTERSYSTEM_ENABLEDSTATE_DISABLED && + hypervDomainShutdown(domain) < 0) { + goto cleanup; + }
Many, but not all, drivers in libvirt allow undefining the cofig for a running VM. ie we can delete the config on disk, without affecting the running VM. This results in a so called "transient" VM - basically a VM which only exists as long as it is running - virDomainCreateXML creates such a beast, while virDomainDefineXML+virDomainCreate creates a "persistent" VM and starts it.
If hyperv doesn't allow that, then shutting down the VM is likely to be surprising.
I'd suggest we report an error indicating undefine is not permitted for a running VM in such a case.
Hyper-V does not allow a VM's definition to be deleted if it's active, paused, or in state transition.
If I remove the check entirely, it displays an error including Hyper-V's method name, the error code from Hyper-V, and a string that's generated by hypervReturnCodeToString() from hyperv_wmi.c.
For example, `virsh undefine runningvm` would throw the error:
error: Failed to undefine domain runningvm error: internal error: Invocation of DestroySystem returned an error: Invalid state for this operation (32775)
That simplifies the code significantly and provides details (the method name and error code) that users could look up in Microsoft's documentation. However, I don't think it's very clearly phrased. "Domain must not be active or in state transition" with the error code set to VIR_ERR_OPERATION_INVALID produces the following error output:
error: Failed to undefine domain testvm error: Requested operation is not valid: Domain must not be active or in state transition
It's a difference of 18 lines (51 vs 33). The shorter version eliminates the result and computerSystem variables, does not use goto, and only sends a single WMI request to Hyper-V since it does not have to query the VM state before attempting to undefine it.
Querying state ahead of time leaves a race condition, so either just let hyperv fail, or catch the error when it fails and report a different error mesage.
Would you prefer to have simpler code or a custom error message?
I don't mind on that front. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 114 +++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 8e16ff529f..559b60d3df 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -703,6 +703,9 @@ hypervFreePrivate(hypervPrivate **priv) if ((*priv)->caps) virObjectUnref((*priv)->caps); + if ((*priv)->xmlopt) + virObjectUnref((*priv)->xmlopt); + hypervFreeParsedUri(&(*priv)->parsedUri); VIR_FREE(*priv); } @@ -735,6 +738,8 @@ hypervInitConnection(virConnectPtr conn, hypervPrivate *priv, } +virDomainDefParserConfig hypervDomainDefParserConfig; + static virDrvOpenStatus hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, virConfPtr conf G_GNUC_UNUSED, @@ -787,6 +792,9 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, if (!priv->caps) goto cleanup; + /* init xmlopt for domain XML */ + priv->xmlopt = virDomainXMLOptionNew(&hypervDomainDefParserConfig, NULL, NULL, NULL, NULL); + conn->privateData = priv; priv = NULL; result = VIR_DRV_OPEN_SUCCESS; @@ -1932,6 +1940,105 @@ hypervDomainUndefine(virDomainPtr domain) } +static virDomainPtr +hypervDomainDefineXML(virConnectPtr conn, const char *xml) +{ + hypervPrivate *priv = conn->privateData; + g_autoptr(virDomainDef) def = NULL; + virDomainPtr domain = NULL; + g_autoptr(hypervInvokeParamsList) params = NULL; + g_autoptr(GHashTable) defineSystemParam = NULL; + + /* parse xml */ + def = virDomainDefParseString(xml, priv->xmlopt, NULL, + 1 << VIR_DOMAIN_VIRT_HYPERV | VIR_DOMAIN_XML_INACTIVE); + + if (!def) + goto error; + + /* abort if a domain with this UUID already exists */ + if ((domain = hypervDomainLookupByUUID(conn, def->uuid))) { + char uuid_string[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(domain->uuid, uuid_string); + virReportError(VIR_ERR_DOM_EXIST, _("Domain already exists with UUID '%s'"), uuid_string); + + // Don't use the 'exit' label, since we don't want to delete the existing domain. + virObjectUnref(domain); + return NULL; + } + + /* prepare params: only set the VM's name for now */ + params = hypervCreateInvokeParamsList("DefineSystem", + MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR, + Msvm_VirtualSystemManagementService_WmiInfo); + + if (!params) + goto error; + + defineSystemParam = hypervCreateEmbeddedParam(Msvm_VirtualSystemSettingData_WmiInfo); + + if (hypervSetEmbeddedProperty(defineSystemParam, "ElementName", def->name) < 0) + goto error; + + if (hypervAddEmbeddedParam(params, "SystemSettings", + &defineSystemParam, Msvm_VirtualSystemSettingData_WmiInfo) < 0) + goto error; + + /* create the VM */ + if (hypervInvokeMethod(priv, ¶ms, NULL) < 0) + goto error; + + /* populate a domain ptr so that we can edit it */ + domain = hypervDomainLookupByName(conn, def->name); + + /* set domain vcpus */ + if (def->vcpus && hypervDomainSetVcpus(domain, def->maxvcpus) < 0) + goto error; + + /* set VM maximum memory */ + if (def->mem.max_memory > 0 && hypervDomainSetMaxMemory(domain, def->mem.max_memory) < 0) + goto error; + + /* set VM memory */ + if (def->mem.cur_balloon > 0 && hypervDomainSetMemory(domain, def->mem.cur_balloon) < 0) + goto error; + + return domain; + + error: + VIR_DEBUG("Domain creation failed, rolling back"); + if (domain) + hypervDomainUndefine(domain); + + return NULL; +} + + +static virDomainPtr +hypervDomainCreateXML(virConnectPtr conn, const char *xmlDesc, unsigned int flags) +{ + virDomainPtr domain; + + virCheckFlags(VIR_DOMAIN_START_PAUSED, NULL); + + /* create the new domain */ + domain = hypervDomainDefineXML(conn, xmlDesc); + if (!domain) + return NULL; + + /* start the domain */ + if (hypervInvokeMsvmComputerSystemRequestStateChange(domain, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED) < 0) + return domain; + + /* If VIR_DOMAIN_START_PAUSED is set, the guest domain will be started, + * but its vCPUs will be paused. */ + if (flags & VIR_DOMAIN_START_PAUSED) + hypervDomainSuspend(domain); + + return domain; +} + + static int hypervDomainGetAutostart(virDomainPtr domain, int *autostart) { @@ -2515,6 +2622,7 @@ static virHypervisorDriver hypervHypervisorDriver = { .connectListDomains = hypervConnectListDomains, /* 0.9.5 */ .connectNumOfDomains = hypervConnectNumOfDomains, /* 0.9.5 */ .connectListAllDomains = hypervConnectListAllDomains, /* 0.10.2 */ + .domainCreateXML = hypervDomainCreateXML, /* 6.10.0 */ .domainLookupByID = hypervDomainLookupByID, /* 0.9.5 */ .domainLookupByUUID = hypervDomainLookupByUUID, /* 0.9.5 */ .domainLookupByName = hypervDomainLookupByName, /* 0.9.5 */ @@ -2543,6 +2651,7 @@ static virHypervisorDriver hypervHypervisorDriver = { .connectNumOfDefinedDomains = hypervConnectNumOfDefinedDomains, /* 0.9.5 */ .domainCreate = hypervDomainCreate, /* 0.9.5 */ .domainCreateWithFlags = hypervDomainCreateWithFlags, /* 0.9.5 */ + .domainDefineXML = hypervDomainDefineXML, /* 6.10.0 */ .domainUndefine = hypervDomainUndefine, /* 6.10.0 */ .domainUndefineFlags = hypervDomainUndefineFlags, /* 6.10.0 */ .domainGetAutostart = hypervDomainGetAutostart, /* 6.9.0 */ @@ -2564,6 +2673,11 @@ static virHypervisorDriver hypervHypervisorDriver = { }; +virDomainDefParserConfig hypervDomainDefParserConfig = { + .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG, +}; + + static void hypervDebugHandler(const char *message, debug_level_e level, void *user_data G_GNUC_UNUSED) -- 2.27.0
On Tue, Nov 24, 2020 at 02:48:32PM -0500, Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 114 +++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 8e16ff529f..559b60d3df 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -703,6 +703,9 @@ hypervFreePrivate(hypervPrivate **priv) if ((*priv)->caps) virObjectUnref((*priv)->caps);
+ if ((*priv)->xmlopt) + virObjectUnref((*priv)->xmlopt); + hypervFreeParsedUri(&(*priv)->parsedUri); VIR_FREE(*priv); } @@ -735,6 +738,8 @@ hypervInitConnection(virConnectPtr conn, hypervPrivate *priv, }
+virDomainDefParserConfig hypervDomainDefParserConfig; + static virDrvOpenStatus hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, virConfPtr conf G_GNUC_UNUSED, @@ -787,6 +792,9 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, if (!priv->caps) goto cleanup;
+ /* init xmlopt for domain XML */ + priv->xmlopt = virDomainXMLOptionNew(&hypervDomainDefParserConfig, NULL, NULL, NULL, NULL); + conn->privateData = priv; priv = NULL; result = VIR_DRV_OPEN_SUCCESS; @@ -1932,6 +1940,105 @@ hypervDomainUndefine(virDomainPtr domain) }
+static virDomainPtr +hypervDomainDefineXML(virConnectPtr conn, const char *xml) +{ + hypervPrivate *priv = conn->privateData; + g_autoptr(virDomainDef) def = NULL; + virDomainPtr domain = NULL; + g_autoptr(hypervInvokeParamsList) params = NULL; + g_autoptr(GHashTable) defineSystemParam = NULL; + + /* parse xml */ + def = virDomainDefParseString(xml, priv->xmlopt, NULL, + 1 << VIR_DOMAIN_VIRT_HYPERV | VIR_DOMAIN_XML_INACTIVE); + + if (!def) + goto error; + + /* abort if a domain with this UUID already exists */ + if ((domain = hypervDomainLookupByUUID(conn, def->uuid))) { + char uuid_string[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(domain->uuid, uuid_string); + virReportError(VIR_ERR_DOM_EXIST, _("Domain already exists with UUID '%s'"), uuid_string); + + // Don't use the 'exit' label, since we don't want to delete the existing domain. + virObjectUnref(domain); + return NULL; + } + + /* prepare params: only set the VM's name for now */ + params = hypervCreateInvokeParamsList("DefineSystem", + MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR, + Msvm_VirtualSystemManagementService_WmiInfo); + + if (!params) + goto error; + + defineSystemParam = hypervCreateEmbeddedParam(Msvm_VirtualSystemSettingData_WmiInfo); + + if (hypervSetEmbeddedProperty(defineSystemParam, "ElementName", def->name) < 0) + goto error; + + if (hypervAddEmbeddedParam(params, "SystemSettings", + &defineSystemParam, Msvm_VirtualSystemSettingData_WmiInfo) < 0) + goto error; + + /* create the VM */ + if (hypervInvokeMethod(priv, ¶ms, NULL) < 0) + goto error; + + /* populate a domain ptr so that we can edit it */ + domain = hypervDomainLookupByName(conn, def->name); + + /* set domain vcpus */ + if (def->vcpus && hypervDomainSetVcpus(domain, def->maxvcpus) < 0) + goto error; + + /* set VM maximum memory */ + if (def->mem.max_memory > 0 && hypervDomainSetMaxMemory(domain, def->mem.max_memory) < 0) + goto error; + + /* set VM memory */ + if (def->mem.cur_balloon > 0 && hypervDomainSetMemory(domain, def->mem.cur_balloon) < 0) + goto error; + + return domain; + + error: + VIR_DEBUG("Domain creation failed, rolling back"); + if (domain) + hypervDomainUndefine(domain); + + return NULL; +} + + +static virDomainPtr +hypervDomainCreateXML(virConnectPtr conn, const char *xmlDesc, unsigned int flags) +{ + virDomainPtr domain; + + virCheckFlags(VIR_DOMAIN_START_PAUSED, NULL); + + /* create the new domain */ + domain = hypervDomainDefineXML(conn, xmlDesc); + if (!domain) + return NULL; + + /* start the domain */ + if (hypervInvokeMsvmComputerSystemRequestStateChange(domain, MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED) < 0) + return domain;
The virDomainCreateXML call is for creating so called trasient guests, which disappear when shutoff. ie there is no persistent config file present when not running. If this isn't supported, then don't implement this method. Only implement the virDomainCreate call, which pairs with virDomainDefineXML.
+ + /* If VIR_DOMAIN_START_PAUSED is set, the guest domain will be started, + * but its vCPUs will be paused. */ + if (flags & VIR_DOMAIN_START_PAUSED) + hypervDomainSuspend(domain);
Hmm, racy. The VIR_DOMAIN_START_PAUSED flag is intended to provide a race free mechanism. If the caller can tolerate faces, then they can just use virDomainSuspend directly after starting it. IOW, if we can't provide race-free behaviour we should reject use of VIR_DOMAIN_START_PAUSED. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_wmi.c | 52 +++++++++++++++++++++++++++++++++++++++++ src/hyperv/hyperv_wmi.h | 5 ++++ 2 files changed, 57 insertions(+) diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index 466296fe2a..4bace10874 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -1585,6 +1585,58 @@ hypervGetStorageAllocationSD(hypervPrivate *priv, * Msvm_VirtualSystemManagementService */ +int +hypervMsvmVSMSAddResourceSettings(virDomainPtr domain, + GHashTable **resourceSettingsPtr, + hypervWmiClassInfoPtr wmiInfo, + WsXmlDocH *response) +{ + int result = -1; + hypervPrivate *priv = domain->conn->privateData; + char uuid_string[VIR_UUID_STRING_BUFLEN]; + Msvm_VirtualSystemSettingData *vssd = NULL; + GHashTable *resourceSettings = *resourceSettingsPtr; + g_autoptr(hypervInvokeParamsList) params = NULL; + g_auto(virBuffer) eprQuery = VIR_BUFFER_INITIALIZER; + + virUUIDFormat(domain->uuid, uuid_string); + + if (hypervGetMsvmVirtualSystemSettingDataFromUUID(priv, uuid_string, &vssd) < 0) + goto cleanup; + + virBufferEscapeSQL(&eprQuery, + MSVM_VIRTUALSYSTEMSETTINGDATA_WQL_SELECT "WHERE InstanceID='%s'", + vssd->data->InstanceID); + + params = hypervCreateInvokeParamsList("AddResourceSettings", + MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR, + Msvm_VirtualSystemManagementService_WmiInfo); + + if (!params) + goto cleanup; + + if (hypervAddEprParam(params, "AffectedConfiguration", + &eprQuery, Msvm_VirtualSystemSettingData_WmiInfo) < 0) + goto cleanup; + + if (hypervAddEmbeddedParam(params, "ResourceSettings", &resourceSettings, wmiInfo) < 0) { + hypervFreeEmbeddedParam(resourceSettings); + goto cleanup; + } + + if (hypervInvokeMethod(priv, ¶ms, response) < 0) + goto cleanup; + + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *)vssd); + *resourceSettingsPtr = NULL; + + return result; +} + + int hypervMsvmVSMSModifyResourceSettings(hypervPrivate *priv, GHashTable **resourceSettingsPtr, diff --git a/src/hyperv/hyperv_wmi.h b/src/hyperv/hyperv_wmi.h index 31f7e2e3ba..ff96b8063d 100644 --- a/src/hyperv/hyperv_wmi.h +++ b/src/hyperv/hyperv_wmi.h @@ -256,6 +256,11 @@ int hypervGetStorageAllocationSD(hypervPrivate *priv, * Msvm_VirtualSystemManagementService */ +int hypervMsvmVSMSAddResourceSettings(virDomainPtr domain, + GHashTable **resourceSettingsPtr, + hypervWmiClassInfoPtr wmiInfo, + WsXmlDocH *response); + int hypervMsvmVSMSModifyResourceSettings(hypervPrivate *priv, GHashTable **resourceSettingsPtr, hypervWmiClassInfoPtr wmiInfo); -- 2.27.0
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 51 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 559b60d3df..1ad52e598a 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -327,6 +327,53 @@ hypervGetDeviceParentRasdFromDeviceId(const char *parentDeviceId, } +static int +hypervDomainCreateSCSIController(virDomainPtr domain) +{ + g_autoptr(GHashTable) scsiResource = NULL; + g_autofree char *resourceType = NULL; + + resourceType = g_strdup_printf("%d", MSVM_RASD_RESOURCETYPE_PARALLEL_SCSI_HBA); + + VIR_DEBUG("Attaching SCSI Controller"); + + /* prepare embedded param */ + scsiResource = hypervCreateEmbeddedParam(Msvm_ResourceAllocationSettingData_WmiInfo); + if (!scsiResource) + return -1; + + if (hypervSetEmbeddedProperty(scsiResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(scsiResource, "ResourceSubType", + "Microsoft:Hyper-V:Synthetic SCSI Controller") < 0) + return -1; + + /* perform the settings change */ + if (hypervMsvmVSMSAddResourceSettings(domain, &scsiResource, + Msvm_ResourceAllocationSettingData_WmiInfo, NULL) < 0) + return -1; + + return 0; +} + + +static int +hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def) +{ + size_t i = 0; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + continue; + + if (hypervDomainCreateSCSIController(domain) < 0) + return -1; + } + + return 0; +} + /* * Functions for deserializing device entries @@ -2003,6 +2050,10 @@ hypervDomainDefineXML(virConnectPtr conn, const char *xml) if (def->mem.cur_balloon > 0 && hypervDomainSetMemory(domain, def->mem.cur_balloon) < 0) goto error; + /* attach all storage */ + if (hypervDomainAttachStorage(domain, def) < 0) + goto error; + return domain; error: -- 2.27.0
On Tue, Nov 24, 2020 at 02:48:34PM -0500, Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 51 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 559b60d3df..1ad52e598a 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -327,6 +327,53 @@ hypervGetDeviceParentRasdFromDeviceId(const char *parentDeviceId, }
+static int +hypervDomainCreateSCSIController(virDomainPtr domain) +{ + g_autoptr(GHashTable) scsiResource = NULL; + g_autofree char *resourceType = NULL; + + resourceType = g_strdup_printf("%d", MSVM_RASD_RESOURCETYPE_PARALLEL_SCSI_HBA); + + VIR_DEBUG("Attaching SCSI Controller"); + + /* prepare embedded param */ + scsiResource = hypervCreateEmbeddedParam(Msvm_ResourceAllocationSettingData_WmiInfo); + if (!scsiResource) + return -1; + + if (hypervSetEmbeddedProperty(scsiResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(scsiResource, "ResourceSubType", + "Microsoft:Hyper-V:Synthetic SCSI Controller") < 0) + return -1; + + /* perform the settings change */ + if (hypervMsvmVSMSAddResourceSettings(domain, &scsiResource, + Msvm_ResourceAllocationSettingData_WmiInfo, NULL) < 0) + return -1; + + return 0; +} + + +static int +hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def) +{ + size_t i = 0; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + continue; + + if (hypervDomainCreateSCSIController(domain) < 0) + return -1;
You ought to pass def->controllers[i] into this method and validate as many properties as practical. At very least validate the model and report VIR_ERR_CONFIG_UNSUPPORTED for any you can't emulate. Probably ought to reject any info->type which is not VIR_DOMAIN_ADDRESS_TYPE_NONE, since you're not attempting todo any device addressing at this time.
+ } + + return 0; +} +
/* * Functions for deserializing device entries @@ -2003,6 +2050,10 @@ hypervDomainDefineXML(virConnectPtr conn, const char *xml) if (def->mem.cur_balloon > 0 && hypervDomainSetMemory(domain, def->mem.cur_balloon) < 0) goto error;
+ /* attach all storage */ + if (hypervDomainAttachStorage(domain, def) < 0) + goto error; + return domain;
error: -- 2.27.0
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Nov 26, 2020, at 9:42 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
You ought to pass def->controllers[i] into this method and validate as many properties as practical. At very least validate the model and report VIR_ERR_CONFIG_UNSUPPORTED for any you can't emulate.
Hyper-V's SCSI controllers are paravirtualized ("synthetic" in Hyper-V terms). I've been omitting the model setting from my XML files. I can think of three options for how to handle this: 1. only support VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT 2. support VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT and VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO 3. add VIR_DOMAIN_CONTROLLER_MODEL_SCSI_HYPERV and support VIR_DOMAIN_CONTROLLER_MODEL_SCSI_HYPERV, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT, and VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO Adding another virDomainControllerModelSCSI seems unnecessary to me because Hyper-V doesn't support emulating any SCSI controllers; it only has its paravirtualized SCSI functionality. Which do you recommend?
Probably ought to reject any info->type which is not VIR_DOMAIN_ADDRESS_TYPE_NONE, since you're not attempting todo any device addressing at this time.
There are no address settings for Hyper-V SCSI controllers, so this will always be the case. I'll report an error if address settings are provided. Thanks! Matt
On Fri, Jan 08, 2021 at 02:50:25AM -0500, Matt Coleman wrote:
On Nov 26, 2020, at 9:42 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
You ought to pass def->controllers[i] into this method and validate as many properties as practical. At very least validate the model and report VIR_ERR_CONFIG_UNSUPPORTED for any you can't emulate.
Hyper-V's SCSI controllers are paravirtualized ("synthetic" in Hyper-V terms). I've been omitting the model setting from my XML files.
I can think of three options for how to handle this:
1. only support VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT
2. support VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT and VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO
3. add VIR_DOMAIN_CONTROLLER_MODEL_SCSI_HYPERV and support VIR_DOMAIN_CONTROLLER_MODEL_SCSI_HYPERV, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT, and VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO
Adding another virDomainControllerModelSCSI seems unnecessary to me because Hyper-V doesn't support emulating any SCSI controllers; it only has its paravirtualized SCSI functionality.
Which do you recommend?
IIRC "DEFAULT" is when no model=... is given at all and usually a driver would expand that into the suitable SCSI controller model for the guest. With that in mind I'd suggest (2).
Probably ought to reject any info->type which is not VIR_DOMAIN_ADDRESS_TYPE_NONE, since you're not attempting todo any device addressing at this time.
There are no address settings for Hyper-V SCSI controllers, so this will always be the case. I'll report an error if address settings are provided.
Thanks! Matt
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 244 +++++++++++++++++++++++++++++++- src/hyperv/hyperv_wmi_classes.h | 1 + 2 files changed, 242 insertions(+), 3 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 1ad52e598a..a3da2ec524 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -327,6 +327,30 @@ hypervGetDeviceParentRasdFromDeviceId(const char *parentDeviceId, } +static char * +hypervGetInstanceIDFromXMLResponse(WsXmlDocH response) +{ + WsXmlNodeH envelope = NULL; + char *instanceId = NULL; + + envelope = ws_xml_get_soap_envelope(response); + if (!envelope) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid XML response")); + return NULL; + } + + instanceId = ws_xml_get_xpath_value(response, (char *)"//w:Selector[@Name='InstanceID']"); + + if (!instanceId) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not find selectors in method response")); + return NULL; + } + + return instanceId; +} + + static int hypervDomainCreateSCSIController(virDomainPtr domain) { @@ -359,10 +383,175 @@ hypervDomainCreateSCSIController(virDomainPtr domain) static int -hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def) +hypervDomainAddVirtualDiskParent(virDomainPtr domain, + virDomainDiskDefPtr disk, + Msvm_ResourceAllocationSettingData *controller, + const char *hostname, + WsXmlDocH *response) { + g_autoptr(GHashTable) controllerResource = NULL; + g_autofree char *parentInstanceIDEscaped = NULL; + g_autofree char *parent__PATH = NULL; + g_autofree char *addressString = g_strdup_printf("%u", disk->info.addr.drive.unit); + g_autofree char *resourceType = NULL; + + resourceType = g_strdup_printf("%d", MSVM_RASD_RESOURCETYPE_DISK_DRIVE); + + controllerResource = hypervCreateEmbeddedParam(Msvm_ResourceAllocationSettingData_WmiInfo); + if (!controllerResource) + return -1; + + parentInstanceIDEscaped = virStringReplace(controller->data->InstanceID, "\\", "\\\\"); + parent__PATH = g_strdup_printf("\\\\%s\\Root\\Virtualization\\V2:" + "Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"", + hostname, parentInstanceIDEscaped); + if (!parent__PATH) + return -1; + + if (hypervSetEmbeddedProperty(controllerResource, "Parent", parent__PATH) < 0) + return -1; + + if (hypervSetEmbeddedProperty(controllerResource, "AddressOnParent", addressString) < 0) + return -1; + + if (hypervSetEmbeddedProperty(controllerResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(controllerResource, "ResourceSubType", + "Microsoft:Hyper-V:Synthetic Disk Drive") < 0) + return -1; + + if (hypervMsvmVSMSAddResourceSettings(domain, &controllerResource, + Msvm_ResourceAllocationSettingData_WmiInfo, + response) < 0) + return -1; + + return 0; +} + + +static int +hypervDomainAddVirtualHardDisk(virDomainPtr domain, + virDomainDiskDefPtr disk, + const char *hostname, + char *parentInstanceID) +{ + g_autoptr(GHashTable) volumeResource = NULL; + g_autofree char *vhdInstanceIdEscaped = NULL; + g_autofree char *vhd__PATH = NULL; + g_autofree char *resourceType = NULL; + + resourceType = g_strdup_printf("%d", MSVM_RASD_RESOURCETYPE_LOGICAL_DISK); + + volumeResource = hypervCreateEmbeddedParam(Msvm_ResourceAllocationSettingData_WmiInfo); + if (!volumeResource) + return -1; + + vhdInstanceIdEscaped = virStringReplace(parentInstanceID, "\\", "\\\\"); + vhd__PATH = g_strdup_printf("\\\\%s\\Root\\Virtualization\\V2:" + "Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"", + hostname, vhdInstanceIdEscaped); + + if (!vhd__PATH) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "Parent", vhd__PATH) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "HostResource", disk->src->path) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "ResourceSubType", + "Microsoft:Hyper-V:Virtual Hard Disk") < 0) + return -1; + + if (hypervMsvmVSMSAddResourceSettings(domain, &volumeResource, + Msvm_ResourceAllocationSettingData_WmiInfo, + NULL) < 0) + return -1; + + return 0; +} + + +static int +hypervDomainAttachVirtualDisk(virDomainPtr domain, + virDomainDiskDefPtr disk, + Msvm_ResourceAllocationSettingData *controller, + const char *hostname) +{ + int result = -1; + g_autofree char *parentInstanceID = NULL; + WsXmlDocH response = NULL; + + VIR_DEBUG("Now attaching disk image '%s' with address %d to bus %d of type %d", + disk->src->path, disk->info.addr.drive.unit, disk->info.addr.drive.controller, disk->bus); + + if (hypervDomainAddVirtualDiskParent(domain, disk, controller, hostname, &response) < 0) + return -1; + + if (!response) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add virtual disk parent")); + return -1; + } + + parentInstanceID = hypervGetInstanceIDFromXMLResponse(response); + if (!parentInstanceID) + goto cleanup; + + if (hypervDomainAddVirtualHardDisk(domain, disk, hostname, parentInstanceID) < 0) + goto cleanup; + + result = 0; + + cleanup: + ws_xml_destroy_doc(response); + + return result; +} + + +static int +hypervDomainAttachStorageVolume(virDomainPtr domain, + virDomainDiskDefPtr disk, + Msvm_ResourceAllocationSettingData *controller, + const char *hostname) +{ + switch (disk->device) { + case VIR_DOMAIN_DISK_DEVICE_DISK: + if (disk->src->type == VIR_STORAGE_TYPE_FILE) + return hypervDomainAttachVirtualDisk(domain, disk, controller, hostname); + else + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported disk type")); + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported disk bus")); + break; + } + + return -1; +} + + +static int +hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def, const char *hostname) +{ + int result = -1; + hypervPrivate *priv = domain->conn->privateData; size_t i = 0; + char uuid_string[VIR_UUID_STRING_BUFLEN]; + int num_scsi_controllers = 0; + int ctrlr_idx = -1; + Msvm_VirtualSystemSettingData *vssd = NULL; + Msvm_ResourceAllocationSettingData *rasd = NULL; + Msvm_ResourceAllocationSettingData *entry = NULL; + Msvm_ResourceAllocationSettingData *ideChannels[HYPERV_MAX_IDE_CHANNELS]; + Msvm_ResourceAllocationSettingData *scsiControllers[HYPERV_MAX_SCSI_CONTROLLERS]; + /* start with attaching scsi controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) continue; @@ -371,7 +560,55 @@ hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def) return -1; } - return 0; + virUUIDFormat(domain->uuid, uuid_string); + + /* filter through all the rasd entries and isolate our controllers */ + if (hypervGetMsvmVirtualSystemSettingDataFromUUID(priv, uuid_string, &vssd) < 0) + goto cleanup; + + if (hypervGetResourceAllocationSD(priv, vssd->data->InstanceID, &rasd) < 0) + goto cleanup; + + entry = rasd; + while (entry) { + if (entry->data->ResourceType == MSVM_RASD_RESOURCETYPE_IDE_CONTROLLER) + ideChannels[entry->data->Address[0] - '0'] = entry; + else if (entry->data->ResourceType == MSVM_RASD_RESOURCETYPE_PARALLEL_SCSI_HBA) + scsiControllers[num_scsi_controllers++] = entry; + + entry = entry->next; + } + + /* now we loop through and attach all the disks */ + for (i = 0; i < def->ndisks; i++) { + switch (def->disks[i]->bus) { + case VIR_DOMAIN_DISK_BUS_IDE: + ctrlr_idx = def->disks[i]->info.addr.drive.bus; + if (hypervDomainAttachStorageVolume(domain, def->disks[i], + ideChannels[ctrlr_idx], hostname) < 0) { + goto cleanup; + } + break; + case VIR_DOMAIN_DISK_BUS_SCSI: + ctrlr_idx = def->disks[i]->info.addr.drive.controller; + if (hypervDomainAttachStorageVolume(domain, def->disks[i], + scsiControllers[ctrlr_idx], hostname) < 0) { + goto cleanup; + } + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported controller type")); + goto cleanup; + } + } + + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *)rasd); + hypervFreeObject(priv, (hypervObject *)vssd); + + return result; } @@ -1991,6 +2228,7 @@ static virDomainPtr hypervDomainDefineXML(virConnectPtr conn, const char *xml) { hypervPrivate *priv = conn->privateData; + g_autofree char *hostname = hypervConnectGetHostname(conn); g_autoptr(virDomainDef) def = NULL; virDomainPtr domain = NULL; g_autoptr(hypervInvokeParamsList) params = NULL; @@ -2051,7 +2289,7 @@ hypervDomainDefineXML(virConnectPtr conn, const char *xml) goto error; /* attach all storage */ - if (hypervDomainAttachStorage(domain, def) < 0) + if (hypervDomainAttachStorage(domain, def, hostname) < 0) goto error; return domain; diff --git a/src/hyperv/hyperv_wmi_classes.h b/src/hyperv/hyperv_wmi_classes.h index 36c0e60c2a..2c03ca3745 100644 --- a/src/hyperv/hyperv_wmi_classes.h +++ b/src/hyperv/hyperv_wmi_classes.h @@ -113,6 +113,7 @@ enum _Msvm_ResourceAllocationSettingData_ResourceType { MSVM_RASD_RESOURCETYPE_DVD_DRIVE = 16, MSVM_RASD_RESOURCETYPE_DISK_DRIVE = 17, MSVM_RASD_RESOURCETYPE_STORAGE_EXTENT = 19, + MSVM_RASD_RESOURCETYPE_LOGICAL_DISK = 31, }; -- 2.27.0
On Tue, Nov 24, 2020 at 02:48:35PM -0500, Matt Coleman wrote:
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 244 +++++++++++++++++++++++++++++++- src/hyperv/hyperv_wmi_classes.h | 1 + 2 files changed, 242 insertions(+), 3 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 1ad52e598a..a3da2ec524 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -327,6 +327,30 @@ hypervGetDeviceParentRasdFromDeviceId(const char *parentDeviceId, }
+static char * +hypervGetInstanceIDFromXMLResponse(WsXmlDocH response) +{ + WsXmlNodeH envelope = NULL; + char *instanceId = NULL; + + envelope = ws_xml_get_soap_envelope(response); + if (!envelope) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid XML response")); + return NULL; + } + + instanceId = ws_xml_get_xpath_value(response, (char *)"//w:Selector[@Name='InstanceID']"); + + if (!instanceId) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not find selectors in method response")); + return NULL; + } + + return instanceId; +} + + static int hypervDomainCreateSCSIController(virDomainPtr domain) { @@ -359,10 +383,175 @@ hypervDomainCreateSCSIController(virDomainPtr domain)
static int -hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def) +hypervDomainAddVirtualDiskParent(virDomainPtr domain, + virDomainDiskDefPtr disk, + Msvm_ResourceAllocationSettingData *controller, + const char *hostname, + WsXmlDocH *response) { + g_autoptr(GHashTable) controllerResource = NULL; + g_autofree char *parentInstanceIDEscaped = NULL; + g_autofree char *parent__PATH = NULL; + g_autofree char *addressString = g_strdup_printf("%u", disk->info.addr.drive.unit);
Validate disk->info.type == DRIVE before accessing this field otherwise the union contents is undefined.
+ g_autofree char *resourceType = NULL; + + resourceType = g_strdup_printf("%d", MSVM_RASD_RESOURCETYPE_DISK_DRIVE); + + controllerResource = hypervCreateEmbeddedParam(Msvm_ResourceAllocationSettingData_WmiInfo); + if (!controllerResource) + return -1; + + parentInstanceIDEscaped = virStringReplace(controller->data->InstanceID, "\\", "\\\\"); + parent__PATH = g_strdup_printf("\\\\%s\\Root\\Virtualization\\V2:" + "Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"", + hostname, parentInstanceIDEscaped); + if (!parent__PATH) + return -1; + + if (hypervSetEmbeddedProperty(controllerResource, "Parent", parent__PATH) < 0) + return -1; + + if (hypervSetEmbeddedProperty(controllerResource, "AddressOnParent", addressString) < 0) + return -1; + + if (hypervSetEmbeddedProperty(controllerResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(controllerResource, "ResourceSubType", + "Microsoft:Hyper-V:Synthetic Disk Drive") < 0) + return -1; + + if (hypervMsvmVSMSAddResourceSettings(domain, &controllerResource, + Msvm_ResourceAllocationSettingData_WmiInfo, + response) < 0) + return -1; + + return 0; +} + + +static int +hypervDomainAddVirtualHardDisk(virDomainPtr domain, + virDomainDiskDefPtr disk, + const char *hostname, + char *parentInstanceID) +{ + g_autoptr(GHashTable) volumeResource = NULL; + g_autofree char *vhdInstanceIdEscaped = NULL; + g_autofree char *vhd__PATH = NULL; + g_autofree char *resourceType = NULL; + + resourceType = g_strdup_printf("%d", MSVM_RASD_RESOURCETYPE_LOGICAL_DISK); + + volumeResource = hypervCreateEmbeddedParam(Msvm_ResourceAllocationSettingData_WmiInfo); + if (!volumeResource) + return -1; + + vhdInstanceIdEscaped = virStringReplace(parentInstanceID, "\\", "\\\\"); + vhd__PATH = g_strdup_printf("\\\\%s\\Root\\Virtualization\\V2:" + "Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"", + hostname, vhdInstanceIdEscaped); + + if (!vhd__PATH) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "Parent", vhd__PATH) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "HostResource", disk->src->path) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "ResourceSubType", + "Microsoft:Hyper-V:Virtual Hard Disk") < 0) + return -1;
The disk->device can be disk, or cdrom or floppy. So if you're hadcoding disk, then validate disk->device matches.
+ + if (hypervMsvmVSMSAddResourceSettings(domain, &volumeResource, + Msvm_ResourceAllocationSettingData_WmiInfo, + NULL) < 0) + return -1; + + return 0; +} + + +static int +hypervDomainAttachVirtualDisk(virDomainPtr domain, + virDomainDiskDefPtr disk, + Msvm_ResourceAllocationSettingData *controller, + const char *hostname) +{ + int result = -1; + g_autofree char *parentInstanceID = NULL; + WsXmlDocH response = NULL; + + VIR_DEBUG("Now attaching disk image '%s' with address %d to bus %d of type %d", + disk->src->path, disk->info.addr.drive.unit, disk->info.addr.drive.controller, disk->bus);
Don't access the disk->info.addr until its type is validated
+ + if (hypervDomainAddVirtualDiskParent(domain, disk, controller, hostname, &response) < 0) + return -1; + + if (!response) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add virtual disk parent")); + return -1; + } + + parentInstanceID = hypervGetInstanceIDFromXMLResponse(response); + if (!parentInstanceID) + goto cleanup; + + if (hypervDomainAddVirtualHardDisk(domain, disk, hostname, parentInstanceID) < 0) + goto cleanup; + + result = 0; + + cleanup: + ws_xml_destroy_doc(response); + + return result; +} + + +static int +hypervDomainAttachStorageVolume(virDomainPtr domain, + virDomainDiskDefPtr disk, + Msvm_ResourceAllocationSettingData *controller, + const char *hostname) +{ + switch (disk->device) { + case VIR_DOMAIN_DISK_DEVICE_DISK: + if (disk->src->type == VIR_STORAGE_TYPE_FILE) + return hypervDomainAttachVirtualDisk(domain, disk, controller, hostname); + else + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported disk type")); + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported disk bus")); + break; + } + + return -1; +} + + +static int +hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def, const char *hostname) +{ + int result = -1; + hypervPrivate *priv = domain->conn->privateData; size_t i = 0; + char uuid_string[VIR_UUID_STRING_BUFLEN]; + int num_scsi_controllers = 0; + int ctrlr_idx = -1; + Msvm_VirtualSystemSettingData *vssd = NULL; + Msvm_ResourceAllocationSettingData *rasd = NULL; + Msvm_ResourceAllocationSettingData *entry = NULL; + Msvm_ResourceAllocationSettingData *ideChannels[HYPERV_MAX_IDE_CHANNELS]; + Msvm_ResourceAllocationSettingData *scsiControllers[HYPERV_MAX_SCSI_CONTROLLERS];
+ /* start with attaching scsi controllers */ for (i = 0; i < def->ncontrollers; i++) { if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) continue; @@ -371,7 +560,55 @@ hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def) return -1; }
- return 0; + virUUIDFormat(domain->uuid, uuid_string); + + /* filter through all the rasd entries and isolate our controllers */ + if (hypervGetMsvmVirtualSystemSettingDataFromUUID(priv, uuid_string, &vssd) < 0) + goto cleanup; + + if (hypervGetResourceAllocationSD(priv, vssd->data->InstanceID, &rasd) < 0) + goto cleanup; + + entry = rasd; + while (entry) { + if (entry->data->ResourceType == MSVM_RASD_RESOURCETYPE_IDE_CONTROLLER) + ideChannels[entry->data->Address[0] - '0'] = entry; + else if (entry->data->ResourceType == MSVM_RASD_RESOURCETYPE_PARALLEL_SCSI_HBA) + scsiControllers[num_scsi_controllers++] = entry; + + entry = entry->next; + } + + /* now we loop through and attach all the disks */ + for (i = 0; i < def->ndisks; i++) { + switch (def->disks[i]->bus) { + case VIR_DOMAIN_DISK_BUS_IDE: + ctrlr_idx = def->disks[i]->info.addr.drive.bus; + if (hypervDomainAttachStorageVolume(domain, def->disks[i], + ideChannels[ctrlr_idx], hostname) < 0) { + goto cleanup; + } + break; + case VIR_DOMAIN_DISK_BUS_SCSI: + ctrlr_idx = def->disks[i]->info.addr.drive.controller; + if (hypervDomainAttachStorageVolume(domain, def->disks[i], + scsiControllers[ctrlr_idx], hostname) < 0) { + goto cleanup; + } + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported controller type")); + goto cleanup; + } + } + + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *)rasd); + hypervFreeObject(priv, (hypervObject *)vssd); + + return result; }
@@ -1991,6 +2228,7 @@ static virDomainPtr hypervDomainDefineXML(virConnectPtr conn, const char *xml) { hypervPrivate *priv = conn->privateData; + g_autofree char *hostname = hypervConnectGetHostname(conn); g_autoptr(virDomainDef) def = NULL; virDomainPtr domain = NULL; g_autoptr(hypervInvokeParamsList) params = NULL; @@ -2051,7 +2289,7 @@ hypervDomainDefineXML(virConnectPtr conn, const char *xml) goto error;
/* attach all storage */ - if (hypervDomainAttachStorage(domain, def) < 0) + if (hypervDomainAttachStorage(domain, def, hostname) < 0) goto error;
return domain; diff --git a/src/hyperv/hyperv_wmi_classes.h b/src/hyperv/hyperv_wmi_classes.h index 36c0e60c2a..2c03ca3745 100644 --- a/src/hyperv/hyperv_wmi_classes.h +++ b/src/hyperv/hyperv_wmi_classes.h @@ -113,6 +113,7 @@ enum _Msvm_ResourceAllocationSettingData_ResourceType { MSVM_RASD_RESOURCETYPE_DVD_DRIVE = 16, MSVM_RASD_RESOURCETYPE_DISK_DRIVE = 17, MSVM_RASD_RESOURCETYPE_STORAGE_EXTENT = 19, + MSVM_RASD_RESOURCETYPE_LOGICAL_DISK = 31, };
-- 2.27.0
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Nov 26, 2020, at 9:48 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Nov 24, 2020 at 02:48:35PM -0500, Matt Coleman wrote:
+ g_autofree char *addressString = g_strdup_printf("%u", disk->info.addr.drive.unit);
Validate disk->info.type == DRIVE before accessing this field otherwise the union contents is undefined.
+ if (hypervSetEmbeddedProperty(volumeResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "ResourceSubType", + "Microsoft:Hyper-V:Virtual Hard Disk") < 0) + return -1;
The disk->device can be disk, or cdrom or floppy. So if you're hadcoding disk, then validate disk->device matches.
The logic in hypervDomainAttachStorageVolume() ensures that it only calls hypervDomainAttachVirtualDisk() if disk->device is equal to VIR_DOMAIN_DISK_DEVICE_DISK. Should I add another check inside hypervDomainAttachVirtualDisk() or is it acceptable to validate within hypervDomainAttachStorageVolume() as long as no other functions call hypervDomainAttachVirtualDisk()? If this is acceptable, I could also validate disk->info.type inside hypervDomainAttachStorageVolume(), which would cover the other two critiques.
+ VIR_DEBUG("Now attaching disk image '%s' with address %d to bus %d of type %d", + disk->src->path, disk->info.addr.drive.unit, disk->info.addr.drive.controller, disk->bus);
Don't access the disk->info.addr until its type is validated
On Fri, Jan 08, 2021 at 05:55:55PM -0500, Matt Coleman wrote:
On Nov 26, 2020, at 9:48 AM, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Tue, Nov 24, 2020 at 02:48:35PM -0500, Matt Coleman wrote:
+ g_autofree char *addressString = g_strdup_printf("%u", disk->info.addr.drive.unit);
Validate disk->info.type == DRIVE before accessing this field otherwise the union contents is undefined.
+ if (hypervSetEmbeddedProperty(volumeResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "ResourceSubType", + "Microsoft:Hyper-V:Virtual Hard Disk") < 0) + return -1;
The disk->device can be disk, or cdrom or floppy. So if you're hadcoding disk, then validate disk->device matches.
The logic in hypervDomainAttachStorageVolume() ensures that it only calls hypervDomainAttachVirtualDisk() if disk->device is equal to VIR_DOMAIN_DISK_DEVICE_DISK.
Should I add another check inside hypervDomainAttachVirtualDisk() or is it acceptable to validate within hypervDomainAttachStorageVolume() as long as no other functions call hypervDomainAttachVirtualDisk()?
As long as its validated somewhere in the flow that's ok. Centralizing validation in the virDomainDefValidateCallback could be something to consider in future. This does validation at the time the XML is parsed, so you can keep the later logic cleaner.
If this is acceptable, I could also validate disk->info.type inside hypervDomainAttachStorageVolume(), which would cover the other two critiques.
+ VIR_DEBUG("Now attaching disk image '%s' with address %d to bus %d of type %d", + disk->src->path, disk->info.addr.drive.unit, disk->info.addr.drive.controller, disk->bus);
Don't access the disk->info.addr until its type is validated
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 111 +++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index a3da2ec524..3d4aa8ab8e 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -514,6 +514,115 @@ hypervDomainAttachVirtualDisk(virDomainPtr domain, } +static int +hypervDomainAttachPhysicalDisk(virDomainPtr domain, + virDomainDiskDefPtr disk, + Msvm_ResourceAllocationSettingData *controller, + const char *hostname) +{ + int result = -1; + hypervPrivate *priv = domain->conn->privateData; + g_autofree char *hostResource = NULL; + g_autofree char *controller__PATH = NULL; + g_auto(GStrv) matches = NULL; + ssize_t found = 0; + g_auto(virBuffer) query = VIR_BUFFER_INITIALIZER; + Msvm_ResourceAllocationSettingData *diskdefault = NULL; + g_autofree char *controllerInstanceIdEscaped = NULL; + g_autoptr(GHashTable) diskResource = NULL; + g_autofree char *addressString = g_strdup_printf("%u", disk->info.addr.drive.unit); + g_autofree char *resourceType = NULL; + + resourceType = g_strdup_printf("%d", MSVM_RASD_RESOURCETYPE_DISK_DRIVE); + + if (strstr(disk->src->path, "NODRIVE")) { + /* Hyper-V doesn't let you define LUNs with no connection */ + VIR_DEBUG("Skipping empty LUN '%s' with address %d on bus %d of type %d", + disk->src->path, disk->info.addr.drive.unit, + disk->info.addr.drive.controller, disk->bus); + return 0; + } + + VIR_DEBUG("Now attaching LUN '%s' with address %d to bus %d of type %d", + disk->src->path, disk->info.addr.drive.unit, + disk->info.addr.drive.controller, disk->bus); + + /* prepare HostResource */ + + /* get Msvm_DiskDrive root device ID */ + virBufferAddLit(&query, + MSVM_RESOURCEALLOCATIONSETTINGDATA_WQL_SELECT + "WHERE ResourceSubType = 'Microsoft:Hyper-V:Physical Disk Drive' " + "AND InstanceID LIKE '%%Default%%'"); + + if (hypervGetWmiClass(Msvm_ResourceAllocationSettingData, &diskdefault) < 0) + return -1; + + if (!diskdefault) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not retrieve default Msvm_DiskDrive object")); + return -1; + } + + found = virStringSearch(diskdefault->data->InstanceID, + "([a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12})", + 1, &matches); + + if (found < 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not get Msvm_DiskDrive default InstanceID")); + goto cleanup; + } + + hostResource = g_strdup_printf("\\\\%s\\Root\\Virtualization\\V2:" + "Msvm_DiskDrive.CreationClassName=\"Msvm_DiskDrive\"," + "DeviceID=\"Microsoft:%s\\\\%s\"," + "SystemCreationClassName=\"Msvm_ComputerSystem\"," + "SystemName=\"%s\"", + hostname, matches[0], disk->src->path, hostname); + + /* create embedded param */ + diskResource = hypervCreateEmbeddedParam(Msvm_ResourceAllocationSettingData_WmiInfo); + if (!diskResource) + return -1; + + controllerInstanceIdEscaped = virStringReplace(controller->data->InstanceID, "\\", "\\\\"); + controller__PATH = g_strdup_printf("\\\\%s\\Root\\Virtualization\\V2:" + "Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"", + hostname, controllerInstanceIdEscaped); + if (!controller__PATH) + goto cleanup; + + if (hypervSetEmbeddedProperty(diskResource, "Parent", controller__PATH) < 0) + goto cleanup; + + if (hypervSetEmbeddedProperty(diskResource, "AddressOnParent", addressString) < 0) + goto cleanup; + + if (hypervSetEmbeddedProperty(diskResource, "ResourceType", resourceType) < 0) + goto cleanup; + + if (hypervSetEmbeddedProperty(diskResource, "ResourceSubType", + "Microsoft:Hyper-V:Physical Disk Drive") < 0) + goto cleanup; + + if (hypervSetEmbeddedProperty(diskResource, "HostResource", hostResource) < 0) + goto cleanup; + + if (hypervMsvmVSMSAddResourceSettings(domain, &diskResource, + Msvm_ResourceAllocationSettingData_WmiInfo, + NULL) < 0) + goto cleanup; + + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *)diskdefault); + + return result; +} + + static int hypervDomainAttachStorageVolume(virDomainPtr domain, virDomainDiskDefPtr disk, @@ -524,6 +633,8 @@ hypervDomainAttachStorageVolume(virDomainPtr domain, case VIR_DOMAIN_DISK_DEVICE_DISK: if (disk->src->type == VIR_STORAGE_TYPE_FILE) return hypervDomainAttachVirtualDisk(domain, disk, controller, hostname); + else if (disk->src->type == VIR_STORAGE_TYPE_BLOCK) + return hypervDomainAttachPhysicalDisk(domain, disk, controller, hostname); else virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported disk type")); break; -- 2.27.0
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 128 +++++++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 3d4aa8ab8e..914f0c743f 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -623,6 +623,132 @@ hypervDomainAttachPhysicalDisk(virDomainPtr domain, } +static int +hypervDomainAddOpticalDrive(virDomainPtr domain, + virDomainDiskDefPtr disk, + Msvm_ResourceAllocationSettingData *controller, + const char *hostname, + WsXmlDocH *response) +{ + g_autoptr(GHashTable) driveResource = NULL; + g_autofree char *parentInstanceIDEscaped = NULL; + g_autofree char *parent__PATH = NULL; + g_autofree char *addressString = g_strdup_printf("%u", disk->info.addr.drive.unit); + g_autofree char *resourceType = NULL; + + resourceType = g_strdup_printf("%d", MSVM_RASD_RESOURCETYPE_DVD_DRIVE); + + driveResource = hypervCreateEmbeddedParam(Msvm_ResourceAllocationSettingData_WmiInfo); + if (!driveResource) + return -1; + + parentInstanceIDEscaped = virStringReplace(controller->data->InstanceID, "\\", "\\\\"); + parent__PATH = g_strdup_printf("\\\\%s\\Root\\Virtualization\\V2:" + "Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"", + hostname, parentInstanceIDEscaped); + if (!parent__PATH) + return -1; + + if (hypervSetEmbeddedProperty(driveResource, "Parent", parent__PATH) < 0) + return -1; + + if (hypervSetEmbeddedProperty(driveResource, "AddressOnParent", addressString) < 0) + return -1; + + if (hypervSetEmbeddedProperty(driveResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(driveResource, "ResourceSubType", + "Microsoft:Hyper-V:Synthetic DVD Drive") < 0) + return -1; + + if (hypervMsvmVSMSAddResourceSettings(domain, &driveResource, + Msvm_ResourceAllocationSettingData_WmiInfo, response) < 0) + return -1; + + return 0; +} + + +static int +hypervDomainAddOpticalDisk(virDomainPtr domain, + virDomainDiskDefPtr disk, + const char *hostname, + char *driveInstanceID) +{ + g_autoptr(GHashTable) volumeResource = NULL; + g_autofree char *vhdInstanceIdEscaped = NULL; + g_autofree char *vhd__PATH = NULL; + g_autofree char *resourceType = NULL; + + resourceType = g_strdup_printf("%d", MSVM_RASD_RESOURCETYPE_LOGICAL_DISK); + + volumeResource = hypervCreateEmbeddedParam(Msvm_ResourceAllocationSettingData_WmiInfo); + if (!volumeResource) + return -1; + + vhdInstanceIdEscaped = virStringReplace(driveInstanceID, "\\", "\\\\"); + vhd__PATH = g_strdup_printf("\\\\%s\\Root\\Virtualization\\V2:" + "Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"", + hostname, vhdInstanceIdEscaped); + if (!vhd__PATH) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "Parent", vhd__PATH) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "HostResource", disk->src->path) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "ResourceSubType", + "Microsoft:Hyper-V:Virtual CD/DVD Disk") < 0) + return -1; + + if (hypervMsvmVSMSAddResourceSettings(domain, &volumeResource, + Msvm_ResourceAllocationSettingData_WmiInfo, NULL) < 0) + return -1; + + return 0; +} + + +static int +hypervDomainAttachCDROM(virDomainPtr domain, + virDomainDiskDefPtr disk, + Msvm_ResourceAllocationSettingData *controller, + const char *hostname) +{ + int result = -1; + WsXmlDocH response = NULL; + g_autofree char *driveInstanceID = NULL; + + VIR_DEBUG("Now attaching CD/DVD '%s' with address %d to bus %d of type %d", + disk->src->path, disk->info.addr.drive.unit, + disk->info.addr.drive.controller, disk->bus); + + if (hypervDomainAddOpticalDrive(domain, disk, controller, hostname, &response) < 0) + goto cleanup; + + driveInstanceID = hypervGetInstanceIDFromXMLResponse(response); + if (!driveInstanceID) + goto cleanup; + + if (hypervDomainAddOpticalDisk(domain, disk, hostname, driveInstanceID) < 0) + goto cleanup; + + result = 0; + + cleanup: + if (response) + ws_xml_destroy_doc(response); + + return result; +} + + static int hypervDomainAttachStorageVolume(virDomainPtr domain, virDomainDiskDefPtr disk, @@ -638,6 +764,8 @@ hypervDomainAttachStorageVolume(virDomainPtr domain, else virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported disk type")); break; + case VIR_DOMAIN_DISK_DEVICE_CDROM: + return hypervDomainAttachCDROM(domain, disk, controller, hostname); default: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported disk bus")); break; -- 2.27.0
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 54 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 914f0c743f..14371eda39 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -749,6 +749,53 @@ hypervDomainAttachCDROM(virDomainPtr domain, } +static int +hypervDomainAttachFloppy(virDomainPtr domain, + virDomainDiskDefPtr disk, + Msvm_ResourceAllocationSettingData *driveSettings, + const char *hostname) +{ + g_autoptr(GHashTable) volumeResource = NULL; + g_autofree char *vhdInstanceIdEscaped = NULL; + g_autofree char *vfd__PATH = NULL; + g_autofree char *resourceType = NULL; + + resourceType = g_strdup_printf("%d", MSVM_RASD_RESOURCETYPE_LOGICAL_DISK); + + volumeResource = hypervCreateEmbeddedParam(Msvm_ResourceAllocationSettingData_WmiInfo); + if (!volumeResource) + return -1; + + vhdInstanceIdEscaped = virStringReplace(driveSettings->data->InstanceID, "\\", "\\\\"); + vfd__PATH = g_strdup_printf("\\\\%s\\Root\\Virtualization\\V2:" + "Msvm_ResourceAllocationSettingData.InstanceID=\"%s\"", + hostname, vhdInstanceIdEscaped); + + if (!vfd__PATH) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "Parent", vfd__PATH) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "HostResource", disk->src->path) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "ResourceType", resourceType) < 0) + return -1; + + if (hypervSetEmbeddedProperty(volumeResource, "ResourceSubType", + "Microsoft:Hyper-V:Virtual Floppy Disk") < 0) + return -1; + + if (hypervMsvmVSMSAddResourceSettings(domain, &volumeResource, + Msvm_ResourceAllocationSettingData_WmiInfo, + NULL) < 0) + return -1; + + return 0; +} + + static int hypervDomainAttachStorageVolume(virDomainPtr domain, virDomainDiskDefPtr disk, @@ -789,6 +836,7 @@ hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def, const char * Msvm_ResourceAllocationSettingData *entry = NULL; Msvm_ResourceAllocationSettingData *ideChannels[HYPERV_MAX_IDE_CHANNELS]; Msvm_ResourceAllocationSettingData *scsiControllers[HYPERV_MAX_SCSI_CONTROLLERS]; + Msvm_ResourceAllocationSettingData *floppySettings = NULL; /* start with attaching scsi controllers */ for (i = 0; i < def->ncontrollers; i++) { @@ -814,6 +862,8 @@ hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def, const char * ideChannels[entry->data->Address[0] - '0'] = entry; else if (entry->data->ResourceType == MSVM_RASD_RESOURCETYPE_PARALLEL_SCSI_HBA) scsiControllers[num_scsi_controllers++] = entry; + else if (entry->data->ResourceType == MSVM_RASD_RESOURCETYPE_DISKETTE_DRIVE) + floppySettings = entry; entry = entry->next; } @@ -835,6 +885,10 @@ hypervDomainAttachStorage(virDomainPtr domain, virDomainDefPtr def, const char * goto cleanup; } break; + case VIR_DOMAIN_DISK_BUS_FDC: + if (hypervDomainAttachFloppy(domain, def->disks[i], floppySettings, hostname) < 0) + goto cleanup; + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported controller type")); goto cleanup; -- 2.27.0
Co-authored-by: Sri Ramanujam <sramanujam@datto.com> Signed-off-by: Matt Coleman <matt@datto.com> --- src/hyperv/hyperv_driver.c | 118 +++++++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 14371eda39..43d84340ec 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -813,6 +813,8 @@ hypervDomainAttachStorageVolume(virDomainPtr domain, break; case VIR_DOMAIN_DISK_DEVICE_CDROM: return hypervDomainAttachCDROM(domain, disk, controller, hostname); + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + return hypervDomainAttachFloppy(domain, disk, controller, hostname); default: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported disk bus")); break; @@ -2621,6 +2623,120 @@ hypervDomainCreateXML(virConnectPtr conn, const char *xmlDesc, unsigned int flag } +static int +hypervDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, unsigned int flags) +{ + int result = -1; + hypervPrivate *priv = domain->conn->privateData; + g_autoptr(virDomainDef) def = NULL; + g_autoptr(virDomainDeviceDef) dev = NULL; + Win32_ComputerSystem *host = NULL; + char *hostname = NULL; + char uuid_string[VIR_UUID_STRING_BUFLEN]; + Msvm_ResourceAllocationSettingData *controller = NULL; + Msvm_ResourceAllocationSettingData *rasd = NULL; + Msvm_ResourceAllocationSettingData *entry = NULL; + Msvm_VirtualSystemSettingData *vssd = NULL; + int num_scsi = 0; + + virCheckFlags(0, -1); + + virUUIDFormat(domain->uuid, uuid_string); + + /* get domain definition */ + if (!(def = virDomainDefNew())) + return result; + + /* get domain device definition */ + dev = virDomainDeviceDefParse(xml, def, priv->xmlopt, NULL, VIR_DOMAIN_DEF_PARSE_INACTIVE); + if (!dev) + goto cleanup; + + /* get the host computer system */ + if (hypervGetPhysicalSystemList(priv, &host) < 0) + goto cleanup; + + hostname = host->data->Name; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + /* get our controller */ + if (hypervGetMsvmVirtualSystemSettingDataFromUUID(priv, uuid_string, &vssd) < 0) + goto cleanup; + + if (hypervGetResourceAllocationSD(priv, vssd->data->InstanceID, &rasd) < 0) + goto cleanup; + + entry = rasd; + switch (dev->data.disk->bus) { + case VIR_DOMAIN_DISK_BUS_IDE: + while (entry) { + if (entry->data->ResourceType == MSVM_RASD_RESOURCETYPE_IDE_CONTROLLER && + (entry->data->Address[0] - '0') == dev->data.disk->info.addr.drive.controller) { + controller = entry; + break; + } + entry = entry->next; + } + if (!entry) + goto cleanup; + break; + case VIR_DOMAIN_DISK_BUS_SCSI: + while (entry) { + if (entry->data->ResourceType == MSVM_RASD_RESOURCETYPE_PARALLEL_SCSI_HBA && + num_scsi++ == dev->data.disk->info.addr.drive.controller) { + controller = entry; + break; + } + entry = entry->next; + } + if (!entry) + goto cleanup; + break; + case VIR_DOMAIN_DISK_BUS_FDC: + while (entry) { + if (entry->data->ResourceType == MSVM_RASD_RESOURCETYPE_DISKETTE_DRIVE) { + controller = entry; + break; + } + entry = entry->next; + } + if (!entry) + goto cleanup; + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid disk bus in definition")); + goto cleanup; + } + + if (hypervDomainAttachStorageVolume(domain, dev->data.disk, controller, hostname) < 0) + goto cleanup; + break; + default: + /* unsupported device type */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Attaching devices of type %d is not implemented"), dev->type); + goto cleanup; + } + + result = 0; + + cleanup: + hypervFreeObject(priv, (hypervObject *)vssd); + hypervFreeObject(priv, (hypervObject *)rasd); + hypervFreeObject(priv, (hypervObject *)host); + + return result; +} + + +static int +hypervDomainAttachDevice(virDomainPtr domain, const char *xml) +{ + return hypervDomainAttachDeviceFlags(domain, xml, 0); +} + + static int hypervDomainGetAutostart(virDomainPtr domain, int *autostart) { @@ -3236,6 +3352,8 @@ static virHypervisorDriver hypervHypervisorDriver = { .domainDefineXML = hypervDomainDefineXML, /* 6.10.0 */ .domainUndefine = hypervDomainUndefine, /* 6.10.0 */ .domainUndefineFlags = hypervDomainUndefineFlags, /* 6.10.0 */ + .domainAttachDevice = hypervDomainAttachDevice, /* 6.10.0 */ + .domainAttachDeviceFlags = hypervDomainAttachDeviceFlags, /* 6.10.0 */ .domainGetAutostart = hypervDomainGetAutostart, /* 6.9.0 */ .domainSetAutostart = hypervDomainSetAutostart, /* 6.9.0 */ .domainGetSchedulerType = hypervDomainGetSchedulerType, /* 6.10.0 */ -- 2.27.0
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 aa8a217eb6..f5b6dfe6a3 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -37,8 +37,11 @@ v6.10.0 (unreleased) ``virDomainGetSchedulerType()``, ``virDomainGetSchedulerParameters()``, ``virDomainGetSchedulerParametersFlags()``, ``virDomainGetVcpus()``, ``virDomainGetVcpusFlags()``, ``virDomainGetMaxVcpus()``, - ``virDomainSetVcpus()``, and ``virDomainSetVcpusFlags()`` APIs have been - implemented in the Hyper-V driver. + ``virDomainSetVcpus()``, ``virDomainSetVcpusFlags()``, + ``virDomainUndefine()``, ``virDomainUndefineFlags()``, + ``virDomainCreateXML()``, ``virDomainDefineXML()``, + ``virDomainAttachDevice()``, and ``virDomainAttachDeviceFlags()`` APIs have + been implemented in the Hyper-V driver. * **Improvements** -- 2.27.0
`git send-email` did something wonky with the subject and cut off "attaching storage devices" -- Matt
participants (2)
-
Daniel P. Berrangé -
Matt Coleman