[libvirt] [PATCH] qemu: Support to detach redirdev device
Attaching redirdev device has been supported for a while, but detaching is not never implemented. Simple procedure to test: % lsusb Bus 001 Device 014: ID 0781:5567 SanDisk Corp. Cruzer Blade % usbredirserver -p 4000 0781:5567 % virsh attach-device test usb.xml % cat usb.xml <redirdev bus='usb' type='tcp'> <source mode='connect' host='192.168.84.6' service='4000'/> </redirdev> % virsh detach-device test usb.xml % virsh qemu-monitor-command test --pretty '{"execute": "query-chardev"}' | grep 4000 On success, the chardev should not seen in output of above command. --- src/conf/domain_conf.c | 67 +++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++ src/libvirt_private.syms | 3 ++ src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 3 ++ 6 files changed, 176 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..d304232 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13825,6 +13825,73 @@ virDomainMemoryRemove(virDomainDefPtr def, return ret; } +ssize_t +virDomainRedirdevFind(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; + + virDomainChrSourceDef source_chr = redirdev->source.chr; + virDomainChrSourceDef tmp_chr = tmp->source.chr; + + if (source_chr.type != tmp_chr.type) + continue; + + switch (source_chr.type) { + case VIR_DOMAIN_CHR_TYPE_TCP: + if (STRNEQ_NULLABLE(source_chr.data.tcp.host, + tmp_chr.data.tcp.host)) + continue; + if (STRNEQ_NULLABLE(source_chr.data.tcp.service, + tmp_chr.data.tcp.service)) + continue; + if (source_chr.data.tcp.listen != tmp_chr.data.tcp.listen) + continue; + if (source_chr.data.tcp.protocol != tmp_chr.data.tcp.protocol) + continue; + break; + + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + if (source_chr.data.spicevmc != tmp_chr.data.spicevmc) + continue; + break; + + default: + /* Unlikely, currently redirdev only supports character device of + * type "tcp" and "spicevmc". + */ + break; + } + + if (redirdev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virDomainDeviceInfoAddressIsEqual(&redirdev->info, &tmp->info)) + continue; + + break; + } + + if (i < def->nredirdevs) + return i; + + return -1; +} + +virDomainRedirdevDefPtr +virDomainRedirdevRemove(virDomainDefPtr def, + size_t idx) +{ + virDomainRedirdevDefPtr ret = def->redirdevs[idx]; + + VIR_DELETE_ELEMENT(def->redirdevs, idx, def->nredirdevs); + + return ret; +} char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1de3be3..03c0155 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2538,6 +2538,10 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainHubDefFree(virDomainHubDefPtr def); void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def); +ssize_t virDomainRedirdevFind(virDomainDefPtr def, + virDomainRedirdevDefPtr redirdev); +virDomainRedirdevDefPtr virDomainRedirdevRemove(virDomainDefPtr def, + size_t idx); void virDomainRedirFilterDefFree(virDomainRedirFilterDefPtr def); void virDomainShmemDefFree(virDomainShmemDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4b40612..ad7d82c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -423,6 +423,9 @@ virDomainPMSuspendedReasonTypeFromString; virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; +virDomainRedirdevDefFree; +virDomainRedirdevFind; +virDomainRedirdevRemove; virDomainRNGBackendTypeToString; virDomainRNGDefFree; virDomainRNGFind; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 45ff3c0..8905af6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7736,6 +7736,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: @@ -7748,7 +7751,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 dc76268..bbe8aa7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3287,6 +3287,49 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, } +static int +qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event; + char *charAlias = NULL; + ssize_t idx; + int rc; + int ret = -1; + + VIR_DEBUG("Removing redirdev device %s from domain %p %s", + redirdev->info.alias, vm, vm->def->name); + + if (virAsprintf(&charAlias, "char%s", redirdev->info.alias) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + virDomainAuditRedirdev(vm, redirdev, "detach", rc == 0); + + if (rc < 0) + goto cleanup; + + event = virDomainEventDeviceRemovedNewFromObj(vm, redirdev->info.alias); + qemuDomainEventQueue(driver, event); + + if ((idx = virDomainRedirdevFind(vm->def, redirdev)) >= 0) + virDomainRedirdevRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &redirdev->info, NULL); + virDomainRedirdevDefFree(redirdev); + + ret = 0; + + cleanup: + VIR_FREE(charAlias); + return ret; +} + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3318,6 +3361,10 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, 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: case VIR_DOMAIN_DEVICE_FS: @@ -3327,7 +3374,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: @@ -4318,3 +4364,52 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, qemuDomainResetDeviceRemoval(vm); return ret; } + +int +qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainRedirdevDefPtr tmp; + ssize_t idx; + int rc; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return -1; + } + + if ((idx = virDomainRedirdevFind(vm->def, redirdev)) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return -1; + } + + tmp = vm->def->redirdevs[idx]; + + if (!tmp->info.alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("alias not set for redirdev device")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &tmp->info); + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDelDevice(priv->mon, tmp->info.alias); + if (qemuDomainObjExitMonitor(driver, vm) || rc < 0) + goto cleanup; + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmp); + else + ret = 0; + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 4140da3..4ef42e9 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -51,6 +51,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRedirdevDefPtr hostdev); +int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev); int qemuDomainAttachHostDevice(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, -- 2.1.4
On 02/27/2016 04:50 AM, Osier Yang wrote:
Attaching redirdev device has been supported for a while, but detaching is not never implemented.
Simple procedure to test:
% lsusb Bus 001 Device 014: ID 0781:5567 SanDisk Corp. Cruzer Blade
% usbredirserver -p 4000 0781:5567
% virsh attach-device test usb.xml
% cat usb.xml <redirdev bus='usb' type='tcp'> <source mode='connect' host='192.168.84.6' service='4000'/> </redirdev>
% virsh detach-device test usb.xml
% virsh qemu-monitor-command test --pretty '{"execute": "query-chardev"}' | grep 4000
On success, the chardev should not seen in output of above command. --- src/conf/domain_conf.c | 67 +++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++ src/libvirt_private.syms | 3 ++ src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 3 ++ 6 files changed, 176 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..d304232 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13825,6 +13825,73 @@ virDomainMemoryRemove(virDomainDefPtr def, return ret; }
A little intro - inputs and what it returns (-1 or the index into the redirdevs for the found redirdev.
+ssize_t +virDomainRedirdevFind(virDomainDefPtr def, + virDomainRedirdevDefPtr redirdev)
I see you're essentially copying the virDomainRNGFind... and friends...
+{ + size_t i; + + for (i = 0; i < def->nredirdevs; i++) { + virDomainRedirdevDefPtr tmp = def->redirdevs[i]; + + if (redirdev->bus != tmp->bus) + continue; + + virDomainChrSourceDef source_chr = redirdev->source.chr; + virDomainChrSourceDef tmp_chr = tmp->source.chr; + + if (source_chr.type != tmp_chr.type) + continue;
Does it matter if the <boot order='#'/> was set in the XML? If it was the boot device, then should it be allowed to be removed? Found yes, but removed? I guess that decision is below us though.
+ + switch (source_chr.type) { + case VIR_DOMAIN_CHR_TYPE_TCP: + if (STRNEQ_NULLABLE(source_chr.data.tcp.host, + tmp_chr.data.tcp.host)) + continue; + if (STRNEQ_NULLABLE(source_chr.data.tcp.service, + tmp_chr.data.tcp.service)) + continue; + if (source_chr.data.tcp.listen != tmp_chr.data.tcp.listen) + continue; + if (source_chr.data.tcp.protocol != tmp_chr.data.tcp.protocol) + continue; + break; + + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + if (source_chr.data.spicevmc != tmp_chr.data.spicevmc) + continue; + break; + + default: + /* Unlikely, currently redirdev only supports character device of + * type "tcp" and "spicevmc". + */
Shouldn't this then be a continue; here? IOW: For anything not being supported we don't want to take the next step, right? I know you're following the RNG code...
+ break; + } + + if (redirdev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
Don't think it matters for checking against TYPE_NONE since the following function does check that... Again more RNG-alike...
+ !virDomainDeviceInfoAddressIsEqual(&redirdev->info, &tmp->info)) + continue;
Should I assume if we get this far then this is *the* device to be removed? And there's only one, right? Hence just "return i;" here (yes, different than rng, more obvious (at least to me) and thus removes the need for "if (i < def->nredirdevs)"
+ + break; + } + + if (i < def->nredirdevs) + return i; + + return -1; +} + +virDomainRedirdevDefPtr +virDomainRedirdevRemove(virDomainDefPtr def, + size_t idx)
I see this code is just a copy of virDomainRNGRemove; however, I'm not convinced that's the best mechanism... The only current caller doesn't check the return value either, although I do note that the RNG code paths to virDomainRNGRemove have a path that would use the returned ret value...
+{ + virDomainRedirdevDefPtr ret = def->redirdevs[idx]; + + VIR_DELETE_ELEMENT(def->redirdevs, idx, def->nredirdevs); + + return ret; +}
char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1de3be3..03c0155 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2538,6 +2538,10 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainHubDefFree(virDomainHubDefPtr def); void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def); +ssize_t virDomainRedirdevFind(virDomainDefPtr def, + virDomainRedirdevDefPtr redirdev); +virDomainRedirdevDefPtr virDomainRedirdevRemove(virDomainDefPtr def, + size_t idx); void virDomainRedirFilterDefFree(virDomainRedirFilterDefPtr def); void virDomainShmemDefFree(virDomainShmemDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4b40612..ad7d82c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -423,6 +423,9 @@ virDomainPMSuspendedReasonTypeFromString; virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; +virDomainRedirdevDefFree; +virDomainRedirdevFind; +virDomainRedirdevRemove; virDomainRNGBackendTypeToString; virDomainRNGDefFree; virDomainRNGFind; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 45ff3c0..8905af6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7736,6 +7736,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
So you're removing from Live, but not Config? Is there a reason? You've followed the RNG code so far...
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: @@ -7748,7 +7751,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 dc76268..bbe8aa7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3287,6 +3287,49 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, }
+static int +qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event; + char *charAlias = NULL; + ssize_t idx; + int rc; + int ret = -1; + + VIR_DEBUG("Removing redirdev device %s from domain %p %s", + redirdev->info.alias, vm, vm->def->name); + + if (virAsprintf(&charAlias, "char%s", redirdev->info.alias) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + virDomainAuditRedirdev(vm, redirdev, "detach", rc == 0); + + if (rc < 0) + goto cleanup; + + event = virDomainEventDeviceRemovedNewFromObj(vm, redirdev->info.alias); + qemuDomainEventQueue(driver, event); + + if ((idx = virDomainRedirdevFind(vm->def, redirdev)) >= 0) + virDomainRedirdevRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &redirdev->info, NULL); + virDomainRedirdevDefFree(redirdev);
There's something inefficient about this... The only reason to call the Find routine is to get the 'idx' value in order to pass to the Remove function which can return a pointer to the redirdev that we already have (and in one path have already gone through the Find logic). I know you're copying RNG, but this device isn't the same as that. Perhaps it'd be better to have a void qemuDomainRedirdevRemove(vm->def, redirdev) to handle all the logic. Also w/r/t qemuDomainReleaseDeviceAddress - I find it interesting that the *Chr* processing handles that in the DetachChrDevice API, while the RNG handles it in RemoveRNG. Additionally, both of those Attach*Device paths have error paths which will call the ReleaseDeviceAddress, but the AttachRedirdevDevice doesn't have similar logic. So the question becomes - is it a required call for this path?
+ + ret = 0; + + cleanup: + VIR_FREE(charAlias); + return ret; +} + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3318,6 +3361,10 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, 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: case VIR_DOMAIN_DEVICE_FS: @@ -3327,7 +3374,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: @@ -4318,3 +4364,52 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, qemuDomainResetDeviceRemoval(vm); return ret; } + +int +qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainRedirdevDefPtr tmp; + ssize_t idx; + int rc; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return -1; + } + + if ((idx = virDomainRedirdevFind(vm->def, redirdev)) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return -1; + } + + tmp = vm->def->redirdevs[idx]; + + if (!tmp->info.alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("alias not set for redirdev device")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &tmp->info); + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDelDevice(priv->mon, tmp->info.alias); + if (qemuDomainObjExitMonitor(driver, vm) || rc < 0)
s/) ||/) < 0 ||/ That is the ExitMonitor needs to check error status... Although I see the RNG device has the same issue <sigh> - need a separate patch for that.
+ goto cleanup; + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmp);
So interestingly the DetachChrDevice will: qemuDomainReleaseDeviceAddress(vm, &tmpChr->info, NULL); ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); But DetachRNG takes a different option; however, I'm still left wondering if it's necessary in this path. John
+ else + ret = 0; + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 4140da3..4ef42e9 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -51,6 +51,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, int qemuDomainAttachRedirdevDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRedirdevDefPtr hostdev); +int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev); int qemuDomainAttachHostDevice(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm,
On 2016年03月05日 19:52, John Ferlan wrote:
On 02/27/2016 04:50 AM, Osier Yang wrote:
Attaching redirdev device has been supported for a while, but detaching is not never implemented.
Simple procedure to test:
% lsusb Bus 001 Device 014: ID 0781:5567 SanDisk Corp. Cruzer Blade
% usbredirserver -p 4000 0781:5567
% virsh attach-device test usb.xml
% cat usb.xml <redirdev bus='usb' type='tcp'> <source mode='connect' host='192.168.84.6' service='4000'/> </redirdev>
% virsh detach-device test usb.xml
% virsh qemu-monitor-command test --pretty '{"execute": "query-chardev"}' | grep 4000
On success, the chardev should not seen in output of above command. --- src/conf/domain_conf.c | 67 +++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 4 ++ src/libvirt_private.syms | 3 ++ src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.h | 3 ++ 6 files changed, 176 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3b15cb4..d304232 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13825,6 +13825,73 @@ virDomainMemoryRemove(virDomainDefPtr def, return ret; }
A little intro - inputs and what it returns (-1 or the index into the redirdevs for the found redirdev.
ok
+ssize_t +virDomainRedirdevFind(virDomainDefPtr def, + virDomainRedirdevDefPtr redirdev) I see you're essentially copying the virDomainRNGFind... and friends...
Honestly, yes. With thinking the existing code is good, I reused them and produced the patch in 1 hour, with making sure the syntax-check and building work well, and the redirdev device can be unhotpluged successfully. You might criticize for why not hack the existing bad code before making a new patch, but I really don't have much time look around currently.
+{ + size_t i; + + for (i = 0; i < def->nredirdevs; i++) { + virDomainRedirdevDefPtr tmp = def->redirdevs[i]; + + if (redirdev->bus != tmp->bus) + continue; + + virDomainChrSourceDef source_chr = redirdev->source.chr; + virDomainChrSourceDef tmp_chr = tmp->source.chr; + + if (source_chr.type != tmp_chr.type) + continue; Does it matter if the <boot order='#'/> was set in the XML? If it was the boot device, then should it be allowed to be removed? Found yes, but removed? I guess that decision is below us though.
Whether the device should be removed or not is not the business of this function, which is just to find the index of the device in the list. And I don't even think libvirt should take care of it.
+ + switch (source_chr.type) { + case VIR_DOMAIN_CHR_TYPE_TCP: + if (STRNEQ_NULLABLE(source_chr.data.tcp.host, + tmp_chr.data.tcp.host)) + continue; + if (STRNEQ_NULLABLE(source_chr.data.tcp.service, + tmp_chr.data.tcp.service)) + continue; + if (source_chr.data.tcp.listen != tmp_chr.data.tcp.listen) + continue; + if (source_chr.data.tcp.protocol != tmp_chr.data.tcp.protocol) + continue; + break; + + case VIR_DOMAIN_CHR_TYPE_SPICEVMC: + if (source_chr.data.spicevmc != tmp_chr.data.spicevmc) + continue; + break; + + default: + /* Unlikely, currently redirdev only supports character device of + * type "tcp" and "spicevmc". + */ Shouldn't this then be a continue; here? IOW: For anything not being supported we don't want to take the next step, right? I know you're following the RNG code...
Well, why to continue? Actually if it flows to this branch, something must be wrong with the @redirdev for which you want to find the index number, either there is bug in the redirdev config parsing, or memory corruption in libvirtd. (Note that the condition for the switch statement is "source_chr.type"). So basically, you can either report error here, or just ignore it with sensible comments. Or better, something like "goto out;", to avoid the below checking (virDomainDeviceInfoAddressIsEqual). If you really think it worths to take care of the situation unlikely happens, I'm fine to refactor it with "goto". Personally I don't think it worths.
+ break; + } + + if (redirdev->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && Don't think it matters for checking against TYPE_NONE since the following function does check that... Again more RNG-alike...
+ !virDomainDeviceInfoAddressIsEqual(&redirdev->info, &tmp->info)) + continue;
Should I assume if we get this far then this is *the* device to be removed? And there's only one, right?
Hence just "return i;" here (yes, different than rng, more obvious (at least to me) and thus removes the need for "if (i < def->nredirdevs)"
Ok
+ + break; + } + + if (i < def->nredirdevs) + return i; + + return -1; +} + +virDomainRedirdevDefPtr +virDomainRedirdevRemove(virDomainDefPtr def, + size_t idx) I see this code is just a copy of virDomainRNGRemove; however, I'm not convinced that's the best mechanism...
The only current caller doesn't check the return value either, although I do note that the RNG code paths to virDomainRNGRemove have a path that would use the returned ret value...
Ok, I will change it to "void".
+{ + virDomainRedirdevDefPtr ret = def->redirdevs[idx]; + + VIR_DELETE_ELEMENT(def->redirdevs, idx, def->nredirdevs); + + return ret; +}
char * virDomainDefGetDefaultEmulator(virDomainDefPtr def, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1de3be3..03c0155 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2538,6 +2538,10 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def); void virDomainHostdevDefFree(virDomainHostdevDefPtr def); void virDomainHubDefFree(virDomainHubDefPtr def); void virDomainRedirdevDefFree(virDomainRedirdevDefPtr def); +ssize_t virDomainRedirdevFind(virDomainDefPtr def, + virDomainRedirdevDefPtr redirdev); +virDomainRedirdevDefPtr virDomainRedirdevRemove(virDomainDefPtr def, + size_t idx); void virDomainRedirFilterDefFree(virDomainRedirFilterDefPtr def); void virDomainShmemDefFree(virDomainShmemDefPtr def); void virDomainDeviceDefFree(virDomainDeviceDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4b40612..ad7d82c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -423,6 +423,9 @@ virDomainPMSuspendedReasonTypeFromString; virDomainPMSuspendedReasonTypeToString; virDomainRedirdevBusTypeFromString; virDomainRedirdevBusTypeToString; +virDomainRedirdevDefFree; +virDomainRedirdevFind; +virDomainRedirdevRemove; virDomainRNGBackendTypeToString; virDomainRNGDefFree; virDomainRNGFind; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 45ff3c0..8905af6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7736,6 +7736,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, So you're removing from Live, but not Config? Is there a reason? You've followed the RNG code so far...
Well, what I want to have is to unhotplug a redirdev device lively, that's the bug I faced. I can post another patch to support to unhotplug it from config though.
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: @@ -7748,7 +7751,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 dc76268..bbe8aa7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3287,6 +3287,49 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, }
+static int +qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virObjectEventPtr event; + char *charAlias = NULL; + ssize_t idx; + int rc; + int ret = -1; + + VIR_DEBUG("Removing redirdev device %s from domain %p %s", + redirdev->info.alias, vm, vm->def->name); + + if (virAsprintf(&charAlias, "char%s", redirdev->info.alias) < 0) + return -1; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + virDomainAuditRedirdev(vm, redirdev, "detach", rc == 0); + + if (rc < 0) + goto cleanup; + + event = virDomainEventDeviceRemovedNewFromObj(vm, redirdev->info.alias); + qemuDomainEventQueue(driver, event); + + if ((idx = virDomainRedirdevFind(vm->def, redirdev)) >= 0) + virDomainRedirdevRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &redirdev->info, NULL); + virDomainRedirdevDefFree(redirdev);
There's something inefficient about this...
The only reason to call the Find routine is to get the 'idx' value in order to pass to the Remove function which can return a pointer to the redirdev that we already have (and in one path have already gone through the Find logic).
I know you're copying RNG, but this device isn't the same as that. Perhaps it'd be better to have a void qemuDomainRedirdevRemove(vm->def, redirdev) to handle all the logic.
Ok
Also w/r/t qemuDomainReleaseDeviceAddress - I find it interesting that the *Chr* processing handles that in the DetachChrDevice API, while the RNG handles it in RemoveRNG. Additionally, both of those Attach*Device paths have error paths which will call the ReleaseDeviceAddress, but the AttachRedirdevDevice doesn't have similar logic. So the question becomes - is it a required call for this path?
I didn't notice the DetachChrDevice releases the device info, but virDomainRedirdevDef has the device info: struct _virDomainRedirdevDef { int bus; /* enum virDomainRedirdevBus */ union { virDomainChrSourceDef chr; } source; virDomainDeviceInfo info; /* Guest address */ }; For why AttachRedirdevDevice doesn't set the device info, honestly, I'm not clear.
+ + ret = 0; + + cleanup: + VIR_FREE(charAlias); + return ret; +} + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3318,6 +3361,10 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, 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: case VIR_DOMAIN_DEVICE_FS: @@ -3327,7 +3374,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: @@ -4318,3 +4364,52 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, qemuDomainResetDeviceRemoval(vm); return ret; } + +int +qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainRedirdevDefPtr tmp; + ssize_t idx; + int rc; + int ret = -1; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("qemu does not support -device")); + return -1; + } + + if ((idx = virDomainRedirdevFind(vm->def, redirdev)) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("device not present in domain configuration")); + return -1; + } + + tmp = vm->def->redirdevs[idx]; + + if (!tmp->info.alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("alias not set for redirdev device")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &tmp->info); + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDelDevice(priv->mon, tmp->info.alias); + if (qemuDomainObjExitMonitor(driver, vm) || rc < 0) s/) ||/) < 0 ||/
Ok. Good catch.
That is the ExitMonitor needs to check error status... Although I see the RNG device has the same issue <sigh> - need a separate patch for that.
+ goto cleanup; + + rc = qemuDomainWaitForDeviceRemoval(vm); + if (rc == 0 || rc == 1) + ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmp); So interestingly the DetachChrDevice will:
qemuDomainReleaseDeviceAddress(vm, &tmpChr->info, NULL); ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr);
But DetachRNG takes a different option; however, I'm still left wondering if it's necessary in this path.
I'm going to wait for the further feedback before making a v2 patch. Thanks for the reviewing Osier
participants (2)
-
John Ferlan -
Osier Yang