There's currently just one limitation: redirdevs that want to go on USB bus require a USB controller, surprisingly. At the same time, since I'm using virDomainDefHasUSB() in this new validator function, it has to be moved a few lines up and also its header needed to be changed a bit: it is now taking a const pointer to domain def since it's not changing anything in there. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 85f6e31..c75279d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4567,14 +4567,45 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) return 0; } +static bool +virDomainDefHasUSB(const virDomainDef *def) +{ + size_t i; + + for (i = 0; i < def->ncontrollers; i++) { + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && + def->controllers[i]->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) + return true; + } + + return false; +} + +static int +virDomainRedirdevDefValidate(const virDomainDef *def, + const virDomainRedirdevDef *redirdev) +{ + if (redirdev->bus == VIR_DOMAIN_REDIRDEV_BUS_USB && + !virDomainDefHasUSB(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Can't add redirected USB device: " + "USB is disabled for this domain")); + return -1; + } + + 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 +4617,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,14 +17090,6 @@ 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")); - virDomainRedirdevDefFree(redirdev); - goto error; - } - def->redirdevs[def->nredirdevs++] = redirdev; } VIR_FREE(nodes); @@ -23578,20 +23600,6 @@ virDomainObjFormat(virDomainXMLOptionPtr xmlopt, } static bool -virDomainDefHasUSB(virDomainDefPtr def) -{ - size_t i; - - for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && - def->controllers[i]->model != VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) - return true; - } - - return false; -} - -static bool virDomainDeviceIsUSB(virDomainDeviceDefPtr dev) { int t = dev->type; -- 2.8.4

On Fri, Jun 10, 2016 at 05:32:55PM +0200, Michal Privoznik wrote: This function returns false either if there is a model='none' USB controller or if there is none at all, but this error was only reported for model="none'. This broke virt-manager's test: https://ci.centos.org/job/virt-manager-test/systems=libvirt-fedora-rawhid... Jan

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(a)redhat.com&gt; --- src/conf/domain_conf.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++++ src/libvirt_private.syms | 2 ++ 3 files changed, 48 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c75279d..e0d10e9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14603,6 +14603,48 @@ 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 (redirdev->info.alias && + STRNEQ_NULLABLE(redirdev->info.alias, tmp->info.alias)) + continue; + + 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 85b9cd1..4625886 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

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(a)redhat.com&gt; --- 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 e0d10e9..11e6ed6 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

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(a)redhat.com&gt; --- 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 efb3f85..33763c7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7794,6 +7794,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainLeaseDefPtr lease; virDomainControllerDefPtr controller; virDomainFSDefPtr fs; + virDomainRedirdevDefPtr redirdev; switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -7911,6 +7912,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: @@ -7921,7 +7930,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

This is fairly simple. We lookup the device in the array of devices and remove it. No magic. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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 33763c7..254da59 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8062,6 +8062,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: @@ -8072,7 +8084,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

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(a)redhat.com&gt; --- 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 254da59..65e96be 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7639,6 +7639,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: @@ -7651,7 +7654,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

The problem is this: when working on redirdev detach, I've noticed that even though I've passed device alias in the input device XML, it got transformed into inactive in qemuDomainDetachDeviceLive() while in qemuDomainDetachDeviceConfig() it was the active version of it. Apparently, if you are doing: virsh # detach-device --config --live domain device.xml our code correctly detects that you want to detach the device from both config and live domain definition. However, due to some internal implementation we need to make a copy of the device that user had provided. And to do that, we call virDomainDeviceDefCopy(). This function basically formats the device into XML and then parse it again. But there's a problem, it's formatting the XML with VIR_DOMAIN_DEF_FORMAT_INACTIVE flag which means that no live data are present in the copy. So we have a (possibly live) device definition that we pass to code detaching it from inactive XML, and inactive device definition that we pass to code detaching it from active XML. This makes no sense. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65e96be..8255d7b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8523,22 +8523,22 @@ qemuDomainDetachDeviceFlags(virDomainPtr dom, if (!vmdef) goto endjob; - if (virDomainDefCompatibleDevice(vmdef, dev, + if (virDomainDefCompatibleDevice(vmdef, dev_copy, VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) goto endjob; - if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev, caps, + if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev_copy, caps, parse_flags, driver->xmlopt)) < 0) goto endjob; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy, + if (virDomainDefCompatibleDevice(vm->def, dev, VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) goto endjob; - if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom)) < 0) + if ((ret = qemuDomainDetachDeviceLive(vm, dev, dom)) < 0) goto endjob; /* * update domain status forcibly because the domain status may be -- 2.8.4