[libvirt] [PATCH 0/9] Redirdev attach & detach
To some extent we have the code in, but this should enable the full support. Michal Privoznik (9): qemuDomainDetachDeviceConfig: make idx type of ssize_t domain_conf: Validate redirdev after parsing Export virDomainRedirdevDefFree virDomainRedirdevDef: Introduce find & remove routines virDomainDeviceInfoIterateInternal: Iterate through redirdevs too qemuMonitorJSONAttachCharDev: Teach spicevmc qemuDomainAttachDeviceConfig: Allow redirdev coldplug qemuDomainDetachDeviceConfig: Allow cold unplug of redirdevs qemuDomainRemoveDevice: Enable live redirdev detach src/conf/domain_conf.c | 84 ++++++++++++++++++++++++++++++++++++++++---- src/conf/domain_conf.h | 4 +++ src/libvirt_private.syms | 3 ++ src/qemu/qemu_driver.c | 29 ++++++++++++--- src/qemu/qemu_hotplug.c | 76 ++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 3 ++ src/qemu/qemu_monitor_json.c | 7 ++++ tests/qemumonitorjsontest.c | 2 +- 8 files changed, 196 insertions(+), 12 deletions(-) -- 2.8.4
The variable is used to hold the index to the device array we are trying to remove. All the functions that set it are expecting it to be type of ssize_t instead of int. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e70d3ce..d7b65f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7951,7 +7951,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainControllerDefPtr cont, det_cont; virDomainChrDefPtr chr; virDomainFSDefPtr fs; - int idx; + ssize_t idx; switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: -- 2.8.4
On Thu, Jun 09, 2016 at 17:02:36 +0200, Michal Privoznik wrote:
The variable is used to hold the index to the device array we are trying to remove. All the functions that set it are expecting it to be type of ssize_t instead of int.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e70d3ce..d7b65f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7951,7 +7951,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainControllerDefPtr cont, det_cont; virDomainChrDefPtr chr; virDomainFSDefPtr fs; - int idx; + ssize_t idx;
A random sample of the "vir*FindBy*" return an integer that is fed to idx and then the functions removing that take a size_t. Use of int here is consistent with the return value of those functions. Are you planing on fixing those too?
On 10.06.2016 09:49, Peter Krempa wrote:
On Thu, Jun 09, 2016 at 17:02:36 +0200, Michal Privoznik wrote:
The variable is used to hold the index to the device array we are trying to remove. All the functions that set it are expecting it to be type of ssize_t instead of int.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e70d3ce..d7b65f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7951,7 +7951,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainControllerDefPtr cont, det_cont; virDomainChrDefPtr chr; virDomainFSDefPtr fs; - int idx; + ssize_t idx;
A random sample of the "vir*FindBy*" return an integer that is fed to idx and then the functions removing that take a size_t. Use of int here is consistent with the return value of those functions.
Are you planing on fixing those too?
Ah, okay; That calls for a bigger fix and thus a separate patch set. I'll drop this one for now then. Michal
On 06/09/2016 11:02 AM, Michal Privoznik wrote:
The variable is used to hold the index to the device array we are trying to remove. All the functions that set it are expecting it to be type of ssize_t instead of int.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e70d3ce..d7b65f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7951,7 +7951,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainControllerDefPtr cont, det_cont; virDomainChrDefPtr chr; virDomainFSDefPtr fs; - int idx; + ssize_t idx;
If you do this, then this code that uses idx will never detect a "not found" condition: if ((idx = virDomainNetFindIdx(vmdef, net)) < 0) return -1; (there are several other similar uses in the same function)
switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK:
On 06/10/2016 08:02 AM, Laine Stump wrote:
On 06/09/2016 11:02 AM, Michal Privoznik wrote:
The variable is used to hold the index to the device array we are trying to remove. All the functions that set it are expecting it to be type of ssize_t instead of int.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e70d3ce..d7b65f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7951,7 +7951,7 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainControllerDefPtr cont, det_cont; virDomainChrDefPtr chr; virDomainFSDefPtr fs; - int idx; + ssize_t idx;
If you do this, then this code that uses idx will never detect a "not found" condition:
if ((idx = virDomainNetFindIdx(vmdef, net)) < 0) return -1;
(there are several other similar uses in the same function)
Ugh. Nevermind. I looked at "ssize_t" and saw "size_t". Too early, not enough caffeine.
There's currently just one limitation: redirdevs that want to go on USB bus require a USB controller, surprisingly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85f6e31..8f5935c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4567,14 +4567,39 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) return 0; } +static int +virDomainRedirdevDefValidate(const virDomainDef *def, + const virDomainRedirdevDef *redirdev) +{ + size_t i; + + if (redirdev->bus != VIR_DOMAIN_REDIRDEV_BUS_USB) + return 0; + + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr controller = def->controllers[i]; + + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Can't add redirected USB device: " + "USB is disabled for this domain")); + } + } + + return 0; +} + static int virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, - const virDomainDef *def ATTRIBUTE_UNUSED) + const virDomainDef *def) { switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: return virDomainDiskDefValidate(dev->data.disk); + case VIR_DOMAIN_DEVICE_REDIRDEV: + return virDomainRedirdevDefValidate(def, dev->data.redirdev); case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_NET: @@ -4586,7 +4611,6 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: - case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: @@ -17060,10 +17084,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (!redirdev) goto error; - if (redirdev->bus == VIR_DOMAIN_REDIRDEV_BUS_USB && usb_none) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Can't add redirected USB device: " - "USB is disabled for this domain")); + if (virDomainRedirdevDefValidate(def, redirdev) < 0) { virDomainRedirdevDefFree(redirdev); goto error; } -- 2.8.4
On Thu, Jun 09, 2016 at 17:02:37 +0200, Michal Privoznik wrote:
There's currently just one limitation: redirdevs that want to go on USB bus require a USB controller, surprisingly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85f6e31..8f5935c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4567,14 +4567,39 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) return 0; }
+static int +virDomainRedirdevDefValidate(const virDomainDef *def, + const virDomainRedirdevDef *redirdev) +{ + size_t i; + + if (redirdev->bus != VIR_DOMAIN_REDIRDEV_BUS_USB)
This condition makes it harder to add checks for non-usb redirdevs later.
+ return 0; + + for (i = 0; i < def->ncontrollers; i++) { + virDomainControllerDefPtr controller = def->controllers[i]; + + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) {
We already have a helper to do this called 'virDomainDefHasUSB'
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Can't add redirected USB device: " + "USB is disabled for this domain")); + } + } + + return 0; +} +
static int virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, - const virDomainDef *def ATTRIBUTE_UNUSED) + const virDomainDef *def) { switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: return virDomainDiskDefValidate(dev->data.disk); + case VIR_DOMAIN_DEVICE_REDIRDEV: + return virDomainRedirdevDefValidate(def, dev->data.redirdev); case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_NET: @@ -4586,7 +4611,6 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, case VIR_DOMAIN_DEVICE_CONTROLLER: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: - case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: @@ -17060,10 +17084,7 @@ virDomainDefParseXML(xmlDocPtr xml, if (!redirdev) goto error;
- if (redirdev->bus == VIR_DOMAIN_REDIRDEV_BUS_USB && usb_none) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Can't add redirected USB device: " - "USB is disabled for this domain")); + if (virDomainRedirdevDefValidate(def, redirdev) < 0) {
Ideally this would be removed altogether. Such checks shouldn't be in the parser.
virDomainRedirdevDefFree(redirdev); goto error; }
In the 162efa1a commit the function was introduced, but the commit forgot to update livirt_private.syms accordingly. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 42f664c..fe7a34d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -424,6 +424,7 @@ virDomainPMSuspendedReasonTypeFromString; virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; +virDomainRedirdevDefFree; virDomainRNGBackendTypeToString; virDomainRNGDefFree; virDomainRNGFind; -- 2.8.4
On Thu, Jun 09, 2016 at 17:02:38 +0200, Michal Privoznik wrote:
In the 162efa1a commit the function was introduced, but the commit forgot to update livirt_private.syms accordingly.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + 1 file changed, 1 insertion(+)
ACK
Basically, there are just two functions introduced here: virDomainRedirdevDefFind which looks up given redirdev in domain definition, and virDomainRedirdevDefRemove which removes the device at given index in the array of devices. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 50 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8f5935c..7b2ff98 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14597,6 +14597,50 @@ virDomainMemoryRemove(virDomainDefPtr def, } +ssize_t +virDomainRedirdevDefFind(virDomainDefPtr def, + virDomainRedirdevDefPtr redirdev) +{ + size_t i; + + for (i = 0; i < def->nredirdevs; i++) { + virDomainRedirdevDefPtr tmp = def->redirdevs[i]; + + if (redirdev->bus != tmp->bus) + continue; + + if (!virDomainChrSourceDefIsEqual(&redirdev->source.chr, + &tmp->source.chr)) + continue; + + if (redirdev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&redirdev->info, &tmp->info)) + continue; + + if (STRNEQ_NULLABLE(redirdev->info.alias, tmp->info.alias)) + continue; + + break; + } + + if (i < def->nredirdevs) + return i; + + return -1; +} + + +virDomainRedirdevDefPtr +virDomainRedirdevDefRemove(virDomainDefPtr def, size_t idx) +{ + virDomainRedirdevDefPtr ret = def->redirdevs[idx]; + + VIR_DELETE_ELEMENT(def->redirdevs, idx, def->nredirdevs); + + return ret; +} + + char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, virCapsPtr caps) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3792562..c1b002a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2827,6 +2827,10 @@ virDomainChrRemove(virDomainDefPtr vmdef, ssize_t virDomainRNGFind(virDomainDefPtr def, virDomainRNGDefPtr rng); virDomainRNGDefPtr virDomainRNGRemove(virDomainDefPtr def, size_t idx); +ssize_t virDomainRedirdevDefFind(virDomainDefPtr def, + virDomainRedirdevDefPtr redirdev); +virDomainRedirdevDefPtr virDomainRedirdevDefRemove(virDomainDefPtr def, size_t idx); + int virDomainSaveXML(const char *configDir, virDomainDefPtr def, const char *xml); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fe7a34d..fc401cd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -424,7 +424,9 @@ virDomainPMSuspendedReasonTypeFromString; virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; +virDomainRedirdevDefFind; virDomainRedirdevDefFree; +virDomainRedirdevDefRemove; virDomainRNGBackendTypeToString; virDomainRNGDefFree; virDomainRNGFind; -- 2.8.4
On Thu, Jun 09, 2016 at 17:02:39 +0200, Michal Privoznik wrote:
Basically, there are just two functions introduced here: virDomainRedirdevDefFind which looks up given redirdev in domain definition, and virDomainRedirdevDefRemove which removes the device at given index in the array of devices.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 50 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8f5935c..7b2ff98 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14597,6 +14597,50 @@ virDomainMemoryRemove(virDomainDefPtr def, }
+ssize_t +virDomainRedirdevDefFind(virDomainDefPtr def, + virDomainRedirdevDefPtr redirdev) +{ + size_t i; + + for (i = 0; i < def->nredirdevs; i++) { + virDomainRedirdevDefPtr tmp = def->redirdevs[i]; + + if (redirdev->bus != tmp->bus) + continue; + + if (!virDomainChrSourceDefIsEqual(&redirdev->source.chr, + &tmp->source.chr)) + continue; + + if (redirdev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&redirdev->info, &tmp->info)) + continue; + + if (STRNEQ_NULLABLE(redirdev->info.alias, tmp->info.alias)) + continue;
I don't think we currently match the alias across our device finding functions. The reason for that is that you won't be able to unplug the device with --live --config specified for virsh or the corresponding flag values as the inactive device doesn't have an alias.
+ + break;
You can "return i" right away since you already found the device.
+ } + + if (i < def->nredirdevs) + return i;
This is then not necessary
+ + return -1; +}
On 10.06.2016 10:06, Peter Krempa wrote:
On Thu, Jun 09, 2016 at 17:02:39 +0200, Michal Privoznik wrote:
Basically, there are just two functions introduced here: virDomainRedirdevDefFind which looks up given redirdev in domain definition, and virDomainRedirdevDefRemove which removes the device at given index in the array of devices.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 50 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8f5935c..7b2ff98 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14597,6 +14597,50 @@ virDomainMemoryRemove(virDomainDefPtr def, }
+ssize_t +virDomainRedirdevDefFind(virDomainDefPtr def, + virDomainRedirdevDefPtr redirdev) +{ + size_t i; + + for (i = 0; i < def->nredirdevs; i++) { + virDomainRedirdevDefPtr tmp = def->redirdevs[i]; + + if (redirdev->bus != tmp->bus) + continue; + + if (!virDomainChrSourceDefIsEqual(&redirdev->source.chr, + &tmp->source.chr)) + continue; + + if (redirdev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&redirdev->info, &tmp->info)) + continue; + + if (STRNEQ_NULLABLE(redirdev->info.alias, tmp->info.alias)) + continue;
I don't think we currently match the alias across our device finding functions. The reason for that is that you won't be able to unplug the device with --live --config specified for virsh or the corresponding flag values as the inactive device doesn't have an alias.
The problem here is that in some cases alias is the only way of differentiating two redirdevs: <redirdev bus='usb' type='spicevmc'> <alias name='redir0'/> </redirdev> <redirdev bus='usb' type='spicevmc'> <alias name='redir1'/> </redirdev> There is no address, nothing. So what do you suggest to do in this case? I came up with comparing aliases. Michal
On Fri, Jun 10, 2016 at 10:30:29 +0200, Michal Privoznik wrote:
On 10.06.2016 10:06, Peter Krempa wrote:
On Thu, Jun 09, 2016 at 17:02:39 +0200, Michal Privoznik wrote:
Basically, there are just two functions introduced here: virDomainRedirdevDefFind which looks up given redirdev in domain definition, and virDomainRedirdevDefRemove which removes the device at given index in the array of devices.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 50 insertions(+)
[...]
I don't think we currently match the alias across our device finding functions. The reason for that is that you won't be able to unplug the device with --live --config specified for virsh or the corresponding flag values as the inactive device doesn't have an alias.
The problem here is that in some cases alias is the only way of differentiating two redirdevs:
<redirdev bus='usb' type='spicevmc'> <alias name='redir0'/> </redirdev> <redirdev bus='usb' type='spicevmc'> <alias name='redir1'/> </redirdev>
There is no address, nothing. So what do you suggest to do in this case? I came up with comparing aliases.
If there's no difference between them, then just pick one at random. Or use similar logic as in virDomainMemoryFindByDefInternal where we match the address only if it was provided by the user. Otherwise we pick one at random from those matching the rest of the data.
This is going to be important later when we received DEVICE_DELETED event on the qemu monitor. If we do, virDomainDefFindDevice() is called to find the device for given device alias in the virDomainDef tree. When we enable removal for redirdevs we need to include them in the lookup process too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7b2ff98..32c3460 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3503,6 +3503,13 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, return -1; } + device.type = VIR_DOMAIN_DEVICE_REDIRDEV; + for (i = 0; i < def->nredirdevs; i++) { + device.data.redirdev = def->redirdevs[i]; + if (cb(def, &device, &def->redirdevs[i]->info, opaque) < 0) + return -1; + } + /* Coverity is not very happy with this - all dead_error_condition */ #if !STATIC_ANALYSIS /* This switch statement is here to trigger compiler warning when adding -- 2.8.4
On Thu, Jun 09, 2016 at 17:02:40 +0200, Michal Privoznik wrote:
This is going to be important later when we received DEVICE_DELETED event on the qemu monitor. If we do, virDomainDefFindDevice() is called to find the device for given device alias in the virDomainDef tree. When we enable removal for redirdevs we need to include them in the lookup process too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7b2ff98..32c3460 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3503,6 +3503,13 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, return -1; }
You need to check here that the VM has USB controllers defined. Otherwise loading of the resulting XML file will fail as there's a check in the parser.
+ device.type = VIR_DOMAIN_DEVICE_REDIRDEV; + for (i = 0; i < def->nredirdevs; i++) { + device.data.redirdev = def->redirdevs[i]; + if (cb(def, &device, &def->redirdevs[i]->info, opaque) < 0) + return -1; + }
On Fri, Jun 10, 2016 at 10:52:47 +0200, Peter Krempa wrote:
On Thu, Jun 09, 2016 at 17:02:40 +0200, Michal Privoznik wrote:
This is going to be important later when we received DEVICE_DELETED event on the qemu monitor. If we do, virDomainDefFindDevice() is called to find the device for given device alias in the virDomainDef tree. When we enable removal for redirdevs we need to include them in the lookup process too.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7b2ff98..32c3460 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3503,6 +3503,13 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, return -1; }
You need to check here that the VM has USB controllers defined. Otherwise loading of the resulting XML file will fail as there's a check in the parser.
Ignore this, this belongs to a completely different patch.
+ device.type = VIR_DOMAIN_DEVICE_REDIRDEV; + for (i = 0; i < def->nredirdevs; i++) { + device.data.redirdev = def->redirdevs[i]; + if (cb(def, &device, &def->redirdevs[i]->info, opaque) < 0) + return -1; + }
ACK
We have the code for attaching redirdevs for ages now. Unfortunately, our monitor code that handles talking to the qemu process was missing a little piece of code that actually enabled the feature. BTW: it really is called "type" on the monitor, even though it's called "name" on the cmd line. Don't ask. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor_json.c | 7 +++++++ tests/qemumonitorjsontest.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 12d2e22..380ddab 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6209,6 +6209,13 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID, break; case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + backend_type = "spicevmc"; + + if (virJSONValueObjectAppendString(data, "type", + virDomainChrSpicevmcTypeToString(chr->data.spicevmc)) < 0) + goto error; + break; + case VIR_DOMAIN_CHR_TYPE_SPICEPORT: case VIR_DOMAIN_CHR_TYPE_PIPE: case VIR_DOMAIN_CHR_TYPE_STDIO: diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 2cd824f..0bcf62a 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -769,7 +769,7 @@ testQemuMonitorJSONAttachChardev(const void *data) CHECK("chr_unix", "{\"return\": {}}"); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_SPICEVMC }; - CHECK_FAIL("chr_spicevmc", "{\"return\": {}}"); + CHECK("chr_spicevmc", "{\"return\": {}}"); chr = (virDomainChrSourceDef) { .type = VIR_DOMAIN_CHR_TYPE_PIPE }; CHECK_FAIL("chr_pipe", "{\"return\": {}}"); -- 2.8.4
On Thu, Jun 09, 2016 at 17:02:41 +0200, Michal Privoznik wrote:
We have the code for attaching redirdevs for ages now. Unfortunately, our monitor code that handles talking to the qemu process was missing a little piece of code that actually enabled the feature.
BTW: it really is called "type" on the monitor, even though it's called "name" on the cmd line. Don't ask.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_monitor_json.c | 7 +++++++ tests/qemumonitorjsontest.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-)
ACK
This is really simple, we just need to append the device into the domain def and that's it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d7b65f1..0baf2c8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7792,6 +7792,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainLeaseDefPtr lease; virDomainControllerDefPtr controller; virDomainFSDefPtr fs; + virDomainRedirdevDefPtr redirdev; switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -7909,6 +7910,14 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, dev->data.memory = NULL; break; + case VIR_DOMAIN_DEVICE_REDIRDEV: + redirdev = dev->data.redirdev; + + if (VIR_APPEND_ELEMENT(vmdef->redirdevs, vmdef->nredirdevs, redirdev) < 0) + return -1; + dev->data.redirdev = NULL; + break; + case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: @@ -7919,7 +7928,6 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_SHMEM: - case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: -- 2.8.4
On Thu, Jun 09, 2016 at 17:02:42 +0200, Michal Privoznik wrote:
This is really simple, we just need to append the device into the domain def and that's it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d7b65f1..0baf2c8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
[...]
@@ -7909,6 +7910,14 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, dev->data.memory = NULL; break;
+ case VIR_DOMAIN_DEVICE_REDIRDEV: + redirdev = dev->data.redirdev; +
You need to check here that the VM has USB controllers defined. Otherwise loading of the resulting XML file will fail as there's a check in the parser.
+ if (VIR_APPEND_ELEMENT(vmdef->redirdevs, vmdef->nredirdevs, redirdev) < 0) + return -1; + dev->data.redirdev = NULL; + break;
On 10.06.2016 10:57, Peter Krempa wrote:
On Thu, Jun 09, 2016 at 17:02:42 +0200, Michal Privoznik wrote:
This is really simple, we just need to append the device into the domain def and that's it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d7b65f1..0baf2c8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
[...]
@@ -7909,6 +7910,14 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, dev->data.memory = NULL; break;
+ case VIR_DOMAIN_DEVICE_REDIRDEV: + redirdev = dev->data.redirdev; +
You need to check here that the VM has USB controllers defined. Otherwise loading of the resulting XML file will fail as there's a check in the parser.
I don't think I need to. I mean, qemuDomainAttachDeviceConfig() is called only from qemuDomainAttachDeviceFlags(). Here, user given XML is parsed by virDomainDeviceDefParse() which at its very end calls virDomainDeviceDefValidate() which in turn calls virDomainDeviceDefValidateInternal(). That brings us to patch 02/09 where I'm changing virDomainDeviceDefValidateInternal() so that it validates redirdevs too. Or have I missed something? Michal
On Fri, Jun 10, 2016 at 15:11:02 +0200, Michal Privoznik wrote:
On 10.06.2016 10:57, Peter Krempa wrote:
On Thu, Jun 09, 2016 at 17:02:42 +0200, Michal Privoznik wrote:
This is really simple, we just need to append the device into the domain def and that's it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d7b65f1..0baf2c8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
[...]
@@ -7909,6 +7910,14 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, dev->data.memory = NULL; break;
+ case VIR_DOMAIN_DEVICE_REDIRDEV: + redirdev = dev->data.redirdev; +
You need to check here that the VM has USB controllers defined. Otherwise loading of the resulting XML file will fail as there's a check in the parser.
I don't think I need to. I mean, qemuDomainAttachDeviceConfig() is called only from qemuDomainAttachDeviceFlags(). Here, user given XML is parsed by virDomainDeviceDefParse() which at its very end calls virDomainDeviceDefValidate() which in turn calls virDomainDeviceDefValidateInternal(). That brings us to patch 02/09 where I'm changing virDomainDeviceDefValidateInternal() so that it validates redirdevs too.
Or have I missed something?
Erm. Apparently I forgot about the patches that I've pushed a few days ago.
This is fairly simple. We lookup the device in the array of devices and remove it. No magic. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0baf2c8..406ce45 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8060,6 +8060,18 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainMemoryDefFree(virDomainMemoryRemove(vmdef, idx)); break; + case VIR_DOMAIN_DEVICE_REDIRDEV: + if ((idx = virDomainRedirdevDefFind(vmdef, + dev->data.redirdev)) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("no matching redirdev was not found")); + return -1; + } + + virDomainRedirdevDefFree(virDomainRedirdevDefRemove(vmdef, idx)); + break; + + case VIR_DOMAIN_DEVICE_INPUT: case VIR_DOMAIN_DEVICE_SOUND: case VIR_DOMAIN_DEVICE_VIDEO: @@ -8070,7 +8082,6 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_SHMEM: - case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: -- 2.8.4
On Thu, Jun 09, 2016 at 17:02:43 +0200, Michal Privoznik wrote:
This is fairly simple. We lookup the device in the array of devices and remove it. No magic.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0baf2c8..406ce45 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
ACK even with necessary tweaks to accomodate prior comments.
For some reason, as soon as redirdev is detached, qemu removes the chardev too. Well, it's easier for us then, so hard feelings here. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++- src/qemu/qemu_hotplug.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 3 ++ 3 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 406ce45..8b27a08 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7637,6 +7637,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_MEMORY: ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory); break; + case VIR_DOMAIN_DEVICE_REDIRDEV: + ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev); + break; case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -7649,7 +7652,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_SHMEM: - case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 58156c6..a7a3675 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3230,6 +3230,34 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, return ret; } +static int +qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev) +{ + virObjectEventPtr event; + ssize_t idx; + + VIR_DEBUG("Removing redirdev device %s from domain %p %s", + redirdev->info.alias, vm, vm->def->name); + + if ((idx = virDomainRedirdevDefFind(vm->def, redirdev)) < 0) + return 0; + + /* QEMU removed the chardev for us already. */ + + virDomainAuditRedirdev(vm, redirdev, "detach", true); + + event = virDomainEventDeviceRemovedNewFromObj(vm, redirdev->info.alias); + qemuDomainEventQueue(driver, event); + + virDomainRedirdevDefRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &redirdev->info, NULL); + virDomainRedirdevDefFree(redirdev); + return 0; +} + + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, @@ -3261,6 +3289,9 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_MEMORY: ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory); break; + case VIR_DOMAIN_DEVICE_REDIRDEV: + ret = qemuDomainRemoveRedirdevDevice(driver, vm, dev->data.redirdev); + break; case VIR_DOMAIN_DEVICE_NONE: case VIR_DOMAIN_DEVICE_LEASE: @@ -3271,7 +3302,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, case VIR_DOMAIN_DEVICE_WATCHDOG: case VIR_DOMAIN_DEVICE_GRAPHICS: case VIR_DOMAIN_DEVICE_HUB: - case VIR_DOMAIN_DEVICE_REDIRDEV: case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: @@ -4172,3 +4202,47 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, qemuDomainResetDeviceRemoval(vm); return ret; } + +int +qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainRedirdevDefPtr tmpRedirdev; + ssize_t idx; + + if ((idx = virDomainRedirdevDefFind(vm->def, redirdev)) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("no matching redirdev was not found")); + return -1; + } + + tmpRedirdev = vm->def->redirdevs[idx]; + + if (!tmpRedirdev->info.alias) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("alias for the redirdev was not found")); + goto cleanup; + } + + qemuDomainMarkDeviceForRemoval(vm, &tmpRedirdev->info); + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorDelDevice(priv->mon, tmpRedirdev->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) { + qemuDomainReleaseDeviceAddress(vm, &tmpRedirdev->info, NULL); + ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmpRedirdev); + } + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 165d345..fc79ee5 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -46,6 +46,9 @@ int qemuDomainAttachNetDevice(virQEMUDriverPtr driver, int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRedirdevDefPtr hostdev); +int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr hostdev); int qemuDomainAttachHostDevice(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, -- 2.8.4
On 06/09/2016 11:02 AM, Michal Privoznik wrote:
To some extent we have the code in, but this should enable the full support.
Michal Privoznik (9): qemuDomainDetachDeviceConfig: make idx type of ssize_t domain_conf: Validate redirdev after parsing Export virDomainRedirdevDefFree virDomainRedirdevDef: Introduce find & remove routines virDomainDeviceInfoIterateInternal: Iterate through redirdevs too qemuMonitorJSONAttachCharDev: Teach spicevmc qemuDomainAttachDeviceConfig: Allow redirdev coldplug qemuDomainDetachDeviceConfig: Allow cold unplug of redirdevs qemuDomainRemoveDevice: Enable live redirdev detach
src/conf/domain_conf.c | 84 ++++++++++++++++++++++++++++++++++++++++---- src/conf/domain_conf.h | 4 +++ src/libvirt_private.syms | 3 ++ src/qemu/qemu_driver.c | 29 ++++++++++++--- src/qemu/qemu_hotplug.c | 76 ++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 3 ++ src/qemu/qemu_monitor_json.c | 7 ++++ tests/qemumonitorjsontest.c | 2 +- 8 files changed, 196 insertions(+), 12 deletions(-)
Note there's an upstream bug for part of this: https://bugzilla.redhat.com/show_bug.cgi?id=1313272 - Cole
participants (4)
-
Cole Robinson -
Laine Stump -
Michal Privoznik -
Peter Krempa