9:02 a.m.
On Fri, Oct 07, 2016 at 01:05:29PM -0400, John Ferlan wrote:
On 09/27/2016 08:24 AM, Martin Kletzander wrote:
> This is needed in order to migrate a domain with shmem devices as that
> is not allowed to migrate.
Sure, but how would anyone know since the migration fails... Not sure
where could/should describe it, but perhaps something nice to be
described somewhere...
Because they'll get "migration with shmem device is not supported"
message when they try it.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 39 +++-
> src/qemu/qemu_hotplug.c | 247 ++++++++++++++++++++-
> src/qemu/qemu_hotplug.h | 6 +
> tests/qemuhotplugtest.c | 21 ++
> .../qemuhotplug-ivshmem-doorbell-detach.xml | 7 +
> .../qemuhotplug-ivshmem-doorbell.xml | 4 +
> .../qemuhotplug-ivshmem-plain-detach.xml | 6 +
> .../qemuhotplug-ivshmem-plain.xml | 3 +
> ...muhotplug-base-live+ivshmem-doorbell-detach.xml | 1 +
> .../qemuhotplug-base-live+ivshmem-doorbell.xml | 65 ++++++
> .../qemuhotplug-base-live+ivshmem-plain-detach.xml | 1 +
> .../qemuhotplug-base-live+ivshmem-plain.xml | 58 +++++
> 12 files changed, 453 insertions(+), 5 deletions(-)
> create mode 100644
tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell-detach.xml
> create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-doorbell.xml
> create mode 100644
tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain-detach.xml
> create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-ivshmem-plain.xml
> create mode 120000
tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell-detach.xml
> create mode 100644
tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-doorbell.xml
> create mode 120000
tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain-detach.xml
> create mode 100644
tests/qemuhotplugtestdomains/qemuhotplug-base-live+ivshmem-plain.xml
Jeez someday I should try to learn how to use these hotplug tests ;-)
I have some reworks of that in the works too, so if you'd like that you
can finish the series ;)
[...]
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 72dd93bbe467..03d75be5e3d7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2242,6 +2242,131 @@ qemuDomainAttachHostDevice(virConnectPtr conn,
> return -1;
> }
>
> +
> +int
> +qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainShmemDefPtr shmem)
> +{
> + int ret = -1;
> + char *shmstr = NULL;
> + char *charAlias = NULL;
> + char *memAlias = NULL;
> + bool release_backing = false;
> + bool release_address = true;
> + virErrorPtr orig_err = NULL;
> + virJSONValuePtr props = NULL;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> + switch ((virDomainShmemModel)shmem->model) {
> + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN:
> + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
This is where I'd expect the capabilities checks to be
Now it's added in the qemuBuildShmemDevStr(), so it will work inherently.
[...]
> @@ -3486,6 +3611,61 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr
driver,
> }
>
>
> +static int
> +qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainShmemDefPtr shmem)
> +{
> + int rc;
> + int ret = -1;
> + ssize_t idx = -1;
> + char *charAlias = NULL;
> + char *memAlias = NULL;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + virObjectEventPtr event = NULL;
> +
> + VIR_DEBUG("Removing shmem device %s from domain %p %s",
> + shmem->info.alias, vm, vm->def->name);
> +
> + if (shmem->server.enabled) {
> + if (virAsprintf(&charAlias, "char%s", shmem->info.alias)
< 0)
> + return -1;
> + } else {
> + if (virAsprintf(&memAlias, "shmmem-%s", shmem->info.alias)
< 0)
> + return -1;
> + }
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> +
> + if (shmem->server.enabled)
> + rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
> + else
> + rc = qemuMonitorDelObject(priv->mon, memAlias);
> +
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
> +
> + virDomainAuditShmem(vm, shmem, "detach", rc == 0);
> +
> + if (rc < 0)
> + goto cleanup;
> +
> + event = virDomainEventDeviceRemovedNewFromObj(vm, shmem->info.alias);
> + qemuDomainEventQueue(driver, event);
> +
> + if ((idx = virDomainShmemDefFind(vm->def, shmem)) >= 0)
> + virDomainShmemDefRemove(vm->def, idx);
> + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL);
I think there be a virDomainShmemDefFree(shmem) here, right? The
virDomainShmemDefRemove only removes shmem from the list
Yes, thanks for noticing.
> @@ -4105,6 +4288,68 @@ int
qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
> return qemuDomainDetachThisHostDevice(driver, vm, detach);
> }
>
> +
> +int
> +qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainDeviceDefPtr dev)
> +{
> + int ret = -1;
> + ssize_t idx = -1;
> + virErrorPtr orig_err = NULL;
> + virDomainShmemDefPtr shmem = NULL;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> + if ((idx = virDomainShmemDefFind(vm->def, dev->data.shmem) < 0)) {
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("device not present in domain configuration"));
> + return -1;
> + }
> +
> + shmem = vm->def->shmems[idx];
> +
> + switch ((virDomainShmemModel)shmem->model) {
> + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN:
> + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
> + break;
> +
> + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM:
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("live detach of shmem model '%s' is not
supported"),
> + virDomainShmemModelTypeToString(shmem->model));
> + /* fall-through */
> + case VIR_DOMAIN_SHMEM_MODEL_LAST:
> + return -1;
> + }
> +
> + qemuDomainMarkDeviceForRemoval(vm, &shmem->info);
> + qemuDomainObjEnterMonitor(driver, vm);
> +
> + ret = qemuMonitorDelDevice(priv->mon, shmem->info.alias);
> +
> + if (ret < 0)
> + orig_err = virSaveLastError();
> +
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + ret = -1;
> +
> + if (ret == 0) {
> + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) {
> + qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL);
> + ret = qemuDomainRemoveDevice(driver, vm, dev);
Why not a direct qemuDomainRemoveShmemDevice(driver, vm, shmem);
It's the pattern other code uses - just concern over the difference - it
does result in the same call eventually.
What other code? It doesn't necessarily result in the same call every
time. That's what qemuDomainWaitForDeviceRemoval() is for. We
shouldn't remove it from the definition if QEMU didn't actually remove
it.
ACK with the capabilities checks and of course being sure we've
free'd
shmem.
good to know, those things will be handled in the next version
Martin