On 12/07/2017 05:42 AM, Chen Hanxiao wrote:

From: Chen Hanxiao <chenhanxiao@gmail.com>

We lacked of hot unplugging redirdev device. This patch add support for it. We could use detach-device --live now.

Alter to: Add support for hot unplugging redirdev device which can use the detach-device --live.

Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- v2.1: rebase on master

src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_hotplug.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 3 ++ 3 files changed, 117 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa30b119a..180bd0ebf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7787,6 +7787,9 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_INPUT: ret = qemuDomainDetachInputDevice(vm, dev->data.input); break; + case VIR_DOMAIN_DEVICE_REDIRDEV: + ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev); + break;

case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_SOUND: @@ -7796,7 +7799,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_MEMBALLOON: case VIR_DOMAIN_DEVICE_NVRAM: - 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 24631cb8f..d97aa6051 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4375,6 +4375,74 @@ qemuDomainRemoveInputDevice(virDomainObjPtr vm, return 0; }

Two empty lines

+static int +qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virObjectEventPtr event; + char *charAlias = NULL; + char *tlsAlias = NULL; + char *secAlias = NULL; + ssize_t idx; + int ret = -1; + + VIR_DEBUG("Removing redirdev device %s from domain %p %s", + redirdev->info.alias, vm, vm->def->name); + + if (!(charAlias = qemuAliasChardevFromDevAlias(redirdev->info.alias))) + goto cleanup; + + if (redirdev->source->type == VIR_DOMAIN_CHR_TYPE_TCP && + redirdev->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) { + + if (!(tlsAlias = qemuAliasTLSObjFromSrcAlias(charAlias))) + goto cleanup; + + /* Best shot at this as the secinfo is destroyed after process launch + * and this path does not recreate it. Thus, if the config has the + * secret UUID and we have a serial TCP chardev, then formulate a + * secAlias which we'll attempt to destroy. */ + if (cfg->chardevTLSx509secretUUID && + !(secAlias = qemuDomainGetSecretAESAlias(charAlias, false))) + goto cleanup; + }

Rather than cut-n-paste (again)... I've posted a patch that will generate a qemuDomainDelChardevTLSObjects helper, see: https://www.redhat.com/archives/libvir-list/2017-December/msg00678.html That would change this code as well... I'll still make a few comments though.

+ + qemuDomainObjEnterMonitor(driver, vm); + /* Usually device_del will remove related chardev as well, + * So we don't need to check its return value. + */

How about: DeviceDel from Detach may remove chardev, so we cannot rely on return status to delete TLS chardevs.

+ ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); + + if (tlsAlias) + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); + if (secAlias) + ignore_value(qemuMonitorDelObject(priv->mon, secAlias));

These would go in the helper...

+ + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup;

And right here it's (conditions from above): if (redirdev->source->type == VIR_DOMAIN_CHR_TYPE_TCP && redirdev->source->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES && qemuDomainDelChardevTLSObjects(driver, vm, charAlias) < 0) goto cleanup;

+ + virDomainAuditRedirdev(vm, redirdev, "detach", true); + + event = virDomainEventDeviceRemovedNewFromObj(vm, redirdev->info.alias); + qemuDomainEventQueue(driver, event); + + if ((idx = virDomainRedirdevDefFind(vm->def, redirdev)) >= 0) + virDomainRedirdevDefRemove(vm->def, idx); + qemuDomainReleaseDeviceAddress(vm, &redirdev->info, NULL); + virDomainRedirdevDefFree(redirdev); + + ret = 0; + + cleanup: + VIR_FREE(charAlias); + VIR_FREE(tlsAlias); + VIR_FREE(secAlias);

virObjectUnref(cfg); (of course w/ common function it wouldn't be necessary)

+ return ret; +} +

int qemuDomainRemoveDevice(virQEMUDriverPtr driver, @@ -5112,6 +5180,49 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, return ret; }

Two empty lines

+int +qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev)

Indent is too much for 2nd/3rd args... follow the Shmem code and call this "dev" (or "def" like Input)... IOW: Something short.

+{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainDefPtr vmdef = vm->def;

No need for vmdef - it's just used once

+ virDomainRedirdevDefPtr tmpRedirdevDef;

and this then becomes redirdev since in reality it *is* the one to be found in the domain.

+ ssize_t idx; + + if ((idx = virDomainRedirdevDefFind(vmdef, redirdev)) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("no matching redirdev was not found")); + return -1; + } + + tmpRedirdevDef = vm->def->redirdevs[idx];

You "could" have used vmdef here, but I'll just remove it and access directly.

+ + if (!tmpRedirdevDef->info.alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("alias not set for redirdev device")); + return -1; + } + + qemuDomainMarkDeviceForRemoval(vm, &tmpRedirdevDef->info); + + qemuDomainObjEnterMonitor(driver, vm); + if (qemuMonitorDelDevice(priv->mon, tmpRedirdevDef->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmpRedirdevDef); + + cleanup: + qemuDomainResetDeviceRemoval(vm); + return ret; +} +

int qemuDomainDetachNetDevice(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 3e0d618e0..6c642c4fd 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -125,6 +125,9 @@ int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, int qemuDomainDetachWatchdog(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainWatchdogDefPtr watchdog);

Blank line for readability

+int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr redirdev);

Use @dev (or @def) here for consistency.

int qemuDomainAttachInputDevice(virQEMUDriverPtr driver, virDomainObjPtr vm,

John

news: Add change for hot unplug redirdev On 12/07/2017 05:42 AM, Chen Hanxiao wrote:

From: Chen Hanxiao <chenhanxiao@gmail.com>

Add hot unplug redirdev in news 'New features' section

No need for this part.

Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- v2.1: put news part into a seperate patch

docs/news.xml | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml index 0ec9857e2..ec32dc850 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -35,6 +35,15 @@ <libvirt> <release version="v4.0.0" date="unreleased"> <section title="New features"> + <change> + <summary> + qemu: Add support for hot unplug redirdev device + </summary> + <description> + We've supported hotplug redirdev device, but lacked of + hot unplug feature. Now we can do it by libvirt. + </description>

The <description> probably isn't necessary... Also, I think this belongs in the improvement section and not a new feature since hotplug already existed. John

+ </change> </section> <section title="Improvements"> </section>