The individual qemuDomainDetach*Device() functions will soon be "less functional", since some of the code that is duplicated in 10 of the 12 detach functions is going to be moved into the common qemuDomainDetachDeviceLive(), which calls them all. qemuhotplugtest.c is the only place any of these individual functions is called other than qemuDomainDetachDeviceLive() itself. Fortunately, qemuDomainDetachDeviceLive() provides exactly the functionality needed by the test driver (except that it supports detach of more device types than the test driver has tests for). This patch replaces the calls to qemuDomainDetach(Chr|Shmen|Watchdog|Disk)Device with a single call to the higher level function, allowing us to shift functionality between the lower level functions without breaking the tests. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- Change from V1: remove chunks that changed now-uncalled qemuDomainDetach*() functions to static. That is done in new Patch 05/14. tests/qemuhotplugtest.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 1491c214d0..3eb97d886a 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -147,16 +147,10 @@ testQemuHotplugDetach(virDomainObjPtr vm, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: - ret = qemuDomainDetachDeviceDiskLive(&driver, vm, dev, async); - break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainDetachChrDevice(&driver, vm, dev->data.chr, async); - break; case VIR_DOMAIN_DEVICE_SHMEM: - ret = qemuDomainDetachShmemDevice(&driver, vm, dev->data.shmem, async); - break; case VIR_DOMAIN_DEVICE_WATCHDOG: - ret = qemuDomainDetachWatchdog(&driver, vm, dev->data.watchdog, async); + ret = qemuDomainDetachDeviceLive(vm, dev, &driver, async); break; default: VIR_TEST_VERBOSE("device type '%s' cannot be detached\n", -- 2.20.1

These are no longer called from qemu_driver.c, since the function that called them (qemuDomainDetachDeviceLive()) has been moved to qemu_hotplug.c, and they are no longer called from testqemuhotplug.c because it now just called qemuDomainDetachDeviceLive() instead of all the subordinate functions. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- NEW PATCH IN V2 src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++------------------ src/qemu/qemu_hotplug.h | 51 ----------------------------------------- 2 files changed, 25 insertions(+), 73 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b435ea99ac..50bfd5f4e6 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5389,7 +5389,7 @@ qemuFindDisk(virDomainDefPtr def, const char *dst) return -1; } -int +static int qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev, @@ -5540,10 +5540,11 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm, } } -int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) +static int +qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + bool async) { int idx, ret = -1; virDomainControllerDefPtr detach = NULL; @@ -5601,10 +5602,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, /* search for a hostdev matching dev and detach it */ -int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) +static int +qemuDomainDetachHostDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + bool async) { virDomainHostdevDefPtr hostdev = dev->data.hostdev; virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; @@ -5716,7 +5718,7 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, } -int +static int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainShmemDefPtr dev, @@ -5770,7 +5772,7 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, } -int +static int qemuDomainDetachWatchdog(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainWatchdogDefPtr dev, @@ -5825,7 +5827,7 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, } -int +static int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRedirdevDefPtr dev, @@ -5869,7 +5871,7 @@ qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, } -int +static int qemuDomainDetachNetDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev, @@ -5935,10 +5937,11 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, } -int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainChrDefPtr chr, - bool async) +static int +qemuDomainDetachChrDevice(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainChrDefPtr chr, + bool async) { int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -5992,7 +5995,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, } -int +static int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng, @@ -6038,7 +6041,7 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, } -int +static int qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr memdef, @@ -6086,7 +6089,7 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, } -int +static int qemuDomainDetachInputDevice(virDomainObjPtr vm, virDomainInputDefPtr def, bool async) @@ -6137,7 +6140,7 @@ qemuDomainDetachInputDevice(virDomainObjPtr vm, } -int +static int qemuDomainDetachVsockDevice(virDomainObjPtr vm, virDomainVsockDefPtr dev, bool async) @@ -6173,7 +6176,7 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm, } -int +static int qemuDomainDetachLease(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainLeaseDefPtr lease) diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 10593cf83c..9980c2a756 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -79,10 +79,6 @@ int qemuDomainFindGraphicsIndex(virDomainDefPtr def, int qemuDomainAttachMemory(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainMemoryDefPtr mem); -int qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainMemoryDefPtr memdef, - bool async); int qemuDomainChangeGraphics(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainGraphicsDefPtr dev); @@ -99,35 +95,6 @@ int qemuDomainChangeNetLinkState(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainNetDefPtr dev, int linkstate); -int qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async); -int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async); -int qemuDomainDetachNetDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async); -int qemuDomainDetachHostDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async); -int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainShmemDefPtr dev, - bool async); -int qemuDomainDetachWatchdog(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainWatchdogDefPtr watchdog, - bool async); - -int qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainRedirdevDefPtr dev, - bool async); int qemuDomainAttachInputDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -140,23 +107,12 @@ int qemuDomainAttachVsockDevice(virQEMUDriverPtr driver, int qemuDomainAttachLease(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainLeaseDefPtr lease); -int qemuDomainDetachLease(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainLeaseDefPtr lease); int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr); -int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainChrDefPtr chr, - bool async); int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng); -int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainRNGDefPtr rng, - bool async); int qemuDomainDetachDeviceLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev, @@ -196,11 +152,4 @@ int qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, virBitmapPtr vcpus, bool state); -int qemuDomainDetachInputDevice(virDomainObjPtr vm, - virDomainInputDefPtr def, - bool async); - -int qemuDomainDetachVsockDevice(virDomainObjPtr vm, - virDomainVsockDefPtr dev, - bool async); #endif /* LIBVIRT_QEMU_HOTPLUG_H */ -- 2.20.1

I'm about to add a second virDomainDeviceDef to this function that will point to the actual device in the domain object. "dev" is just a partially filled-in example of what to look for. Naming it match will make the code easier to follow. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- Change from V1: Eliminate movement of Chr and Lease Detach functions, and addition of extra comments in switch. This has been moved to new patch 07/14 src/qemu/qemu_hotplug.c | 32 ++++++++++++++++---------------- src/qemu/qemu_hotplug.h | 2 +- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 50bfd5f4e6..18bf2da264 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6202,52 +6202,52 @@ qemuDomainDetachLease(virQEMUDriverPtr driver, int qemuDomainDetachDeviceLive(virDomainObjPtr vm, - virDomainDeviceDefPtr dev, + virDomainDeviceDefPtr match, virQEMUDriverPtr driver, bool async) { int ret = -1; - switch ((virDomainDeviceType)dev->type) { + switch ((virDomainDeviceType)match->type) { case VIR_DOMAIN_DEVICE_DISK: - ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, async); + ret = qemuDomainDetachDeviceDiskLive(driver, vm, match, async); break; case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainDetachControllerDevice(driver, vm, dev, async); + ret = qemuDomainDetachControllerDevice(driver, vm, match, async); break; case VIR_DOMAIN_DEVICE_LEASE: - ret = qemuDomainDetachLease(driver, vm, dev->data.lease); + ret = qemuDomainDetachLease(driver, vm, match->data.lease); break; case VIR_DOMAIN_DEVICE_NET: - ret = qemuDomainDetachNetDevice(driver, vm, dev, async); + ret = qemuDomainDetachNetDevice(driver, vm, match, async); break; case VIR_DOMAIN_DEVICE_HOSTDEV: - ret = qemuDomainDetachHostDevice(driver, vm, dev, async); + ret = qemuDomainDetachHostDevice(driver, vm, match, async); break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr, async); + ret = qemuDomainDetachChrDevice(driver, vm, match->data.chr, async); break; case VIR_DOMAIN_DEVICE_RNG: - ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng, async); + ret = qemuDomainDetachRNGDevice(driver, vm, match->data.rng, async); break; case VIR_DOMAIN_DEVICE_MEMORY: - ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory, async); + ret = qemuDomainDetachMemoryDevice(driver, vm, match->data.memory, async); break; case VIR_DOMAIN_DEVICE_SHMEM: - ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem, async); + ret = qemuDomainDetachShmemDevice(driver, vm, match->data.shmem, async); break; case VIR_DOMAIN_DEVICE_WATCHDOG: - ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog, async); + ret = qemuDomainDetachWatchdog(driver, vm, match->data.watchdog, async); break; case VIR_DOMAIN_DEVICE_INPUT: - ret = qemuDomainDetachInputDevice(vm, dev->data.input, async); + ret = qemuDomainDetachInputDevice(vm, match->data.input, async); break; case VIR_DOMAIN_DEVICE_REDIRDEV: - ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev, async); + ret = qemuDomainDetachRedirdevDevice(driver, vm, match->data.redirdev, async); break; case VIR_DOMAIN_DEVICE_VSOCK: - ret = qemuDomainDetachVsockDevice(vm, dev->data.vsock, async); + ret = qemuDomainDetachVsockDevice(vm, match->data.vsock, async); break; case VIR_DOMAIN_DEVICE_FS: @@ -6265,7 +6265,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_LAST: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("live detach of device '%s' is not supported"), - virDomainDeviceTypeToString(dev->type)); + virDomainDeviceTypeToString(match->type)); break; } diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 9980c2a756..78aa4e991b 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -115,7 +115,7 @@ int qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, virDomainRNGDefPtr rng); int qemuDomainDetachDeviceLive(virDomainObjPtr vm, - virDomainDeviceDefPtr dev, + virDomainDeviceDefPtr match, virQEMUDriverPtr driver, bool async); -- 2.20.1

The Chr and Lease devices have detach code that is too different from the other device types to handle with common functionality (which will soon be added at the end of qemuDomainDetachDeviceLive(). In order to make this difference obvious, move the cases for those two device types to the top of the switch statement in qemuDomainDetachDeviceLive(), have the cases return immediately so the future common code at the end of the function will be skipped, and also include some hopefully helpful comments to remind future maintainers why these two device types are treated differently. Any attempt to detach an unsupported device type should also skip the future common code at the end of the function, so the case for unsupported types is similarly changed from a simple break to a return -1. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- NEW PATCH IN V2. src/qemu/qemu_hotplug.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 18bf2da264..c9b8868f9c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6209,24 +6209,36 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, int ret = -1; switch ((virDomainDeviceType)match->type) { + /* + * lease and chr devices don't follow the standard pattern of + * the others, so they must have their own self-contained + * Detach functions. + */ + case VIR_DOMAIN_DEVICE_LEASE: + return qemuDomainDetachLease(driver, vm, match->data.lease); + + case VIR_DOMAIN_DEVICE_CHR: + return qemuDomainDetachChrDevice(driver, vm, match->data.chr, async); + + /* + * All the other device types follow a very similar pattern - + * First we call type-specific functions to 1) locate the + * device we want to detach (based on the prototype device in + * match) and 2) do any device-type-specific validation to + * assure it is okay to detach the device. + */ case VIR_DOMAIN_DEVICE_DISK: ret = qemuDomainDetachDeviceDiskLive(driver, vm, match, async); break; case VIR_DOMAIN_DEVICE_CONTROLLER: ret = qemuDomainDetachControllerDevice(driver, vm, match, async); break; - case VIR_DOMAIN_DEVICE_LEASE: - ret = qemuDomainDetachLease(driver, vm, match->data.lease); - break; case VIR_DOMAIN_DEVICE_NET: ret = qemuDomainDetachNetDevice(driver, vm, match, async); break; case VIR_DOMAIN_DEVICE_HOSTDEV: ret = qemuDomainDetachHostDevice(driver, vm, match, async); break; - case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainDetachChrDevice(driver, vm, match->data.chr, async); - break; case VIR_DOMAIN_DEVICE_RNG: ret = qemuDomainDetachRNGDevice(driver, vm, match->data.rng, async); break; @@ -6266,7 +6278,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("live detach of device '%s' is not supported"), virDomainDeviceTypeToString(match->type)); - break; + return -1; } return ret; -- 2.20.1

Most of these functions will soon contain only some setup for detaching the device, not the detach code proper (since that code is identical for these devices). Their device specific functions are all being renamed to qemuDomainDetachPrep*(), where * is the name of that device's data member in the virDomainDeviceDef object. Since there will be other code in qemuDomainDetachDeviceLive() after the calls to qemuDomainDetachPrep*() that could still fail, we no longer directly set "ret" with the return code from qemuDomainDetachPrep*() functions, but simply return -1 on failure, and wait until the end of qemuDomainDetachDeviceLive() to set ret = 0. Along with the rename, qemuDomainDetachPrep*() functions are also given similar arglists, including an arg called "match" that points to the proto-object of the device we want to delete, and another arg "detach" that is used to return a pointer to the actual object that will be (for now *has been*) detached. To make sure these new args aren't confused with existing local pointers that sometimes had the same name (detach), the local pointer to the device is now named after the device type ("controller", "disk", etc). These point to the same place as (*detach)->data.blah, it's just easier on the eyes to have, e.g., "disk->dst" rather than "(*detach)->data.disk-dst". Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- Change from V1: g * Renaming of Chr and Lease functions is now in a separate patch - 09/14. * "reverting name change" made in previous patch" that was pointed out by Peter is eliminated - I removed the name change from the earlier patch prior to pushing, thus simplifying both patches. * preserved NULL initialization of pointers that had it (no matter how unnecessary) * I *haven't* removed ret from qemuDomainDetachDeviceLive() as suggested in review, since it's just going to be added back in Patch 12/14 anyway. src/qemu/qemu_hotplug.c | 317 +++++++++++++++++++++++----------------- 1 file changed, 182 insertions(+), 135 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c9b8868f9c..0c8e673381 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5390,21 +5390,22 @@ qemuFindDisk(virDomainDefPtr def, const char *dst) } static int -qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) +qemuDomainDetachPrepDisk(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr match, + virDomainDiskDefPtr *detach, + bool async) { virDomainDiskDefPtr disk; int idx; int ret = -1; - if ((idx = qemuFindDisk(vm->def, dev->data.disk->dst)) < 0) { + if ((idx = qemuFindDisk(vm->def, match->dst)) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, - _("disk %s not found"), dev->data.disk->dst); + _("disk %s not found"), match->dst); return -1; } - disk = vm->def->disks[idx]; + *detach = disk = vm->def->disks[idx]; switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: @@ -5541,57 +5542,55 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm, } static int -qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) +qemuDomainDetachPrepController(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainControllerDefPtr match, + virDomainControllerDefPtr *detach, + bool async) { int idx, ret = -1; - virDomainControllerDefPtr detach = NULL; + virDomainControllerDefPtr controller = NULL; - if (dev->data.controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + if (match->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("'%s' controller cannot be hot unplugged."), - virDomainControllerTypeToString(dev->data.controller->type)); + virDomainControllerTypeToString(match->type)); return -1; } - if ((idx = virDomainControllerFind(vm->def, - dev->data.controller->type, - dev->data.controller->idx)) < 0) { + if ((idx = virDomainControllerFind(vm->def, match->type, match->idx)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, _("controller %s:%d not found"), - virDomainControllerTypeToString(dev->data.controller->type), - dev->data.controller->idx); + virDomainControllerTypeToString(match->type), + match->idx); goto cleanup; } - detach = vm->def->controllers[idx]; + *detach = controller = vm->def->controllers[idx]; - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { + if (qemuIsMultiFunctionDevice(vm->def, &controller->info)) { virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %s"), - dev->data.disk->dst); + "%s", _("cannot hot unplug multifunction PCI device")); goto cleanup; } - if (qemuDomainControllerIsBusy(vm, detach)) { + if (qemuDomainControllerIsBusy(vm, controller)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("device cannot be detached: device is busy")); goto cleanup; } if (!async) - qemuDomainMarkDeviceForRemoval(vm, &detach->info); + qemuDomainMarkDeviceForRemoval(vm, &controller->info); - if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0) + if (qemuDomainDeleteDevice(vm, controller->info.alias) < 0) goto cleanup; if (async) { ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveControllerDevice(driver, vm, detach); + ret = qemuDomainRemoveControllerDevice(driver, vm, controller); } cleanup: @@ -5603,29 +5602,30 @@ qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, /* search for a hostdev matching dev and detach it */ static int -qemuDomainDetachHostDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) +qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainHostdevDefPtr match, + virDomainHostdevDefPtr *detach, + bool async) { - virDomainHostdevDefPtr hostdev = dev->data.hostdev; - virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; + virDomainHostdevSubsysPtr subsys = &match->source.subsys; virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb; virDomainHostdevSubsysPCIPtr pcisrc = &subsys->u.pci; virDomainHostdevSubsysSCSIPtr scsisrc = &subsys->u.scsi; virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev; - virDomainHostdevDefPtr detach = NULL; + virDomainHostdevDefPtr hostdev = NULL; int idx; int ret = -1; - if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { + if (match->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hot unplug is not supported for hostdev mode '%s'"), - virDomainHostdevModeTypeToString(hostdev->mode)); + virDomainHostdevModeTypeToString(match->mode)); return -1; } - idx = virDomainHostdevFind(vm->def, hostdev, &detach); + idx = virDomainHostdevFind(vm->def, match, &hostdev); + *detach = hostdev; if (idx < 0) { switch (subsys->type) { @@ -5678,27 +5678,27 @@ qemuDomainDetachHostDevice(virQEMUDriverPtr driver, return -1; } - if (qemuIsMultiFunctionDevice(vm->def, detach->info)) { + if (qemuIsMultiFunctionDevice(vm->def, hostdev->info)) { virReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug multifunction PCI device with guest address: " "%.4x:%.2x:%.2x.%.1x"), - detach->info->addr.pci.domain, detach->info->addr.pci.bus, - detach->info->addr.pci.slot, detach->info->addr.pci.function); + hostdev->info->addr.pci.domain, hostdev->info->addr.pci.bus, + hostdev->info->addr.pci.slot, hostdev->info->addr.pci.function); return -1; } - if (!detach->info->alias) { + if (!hostdev->info->alias) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("device cannot be detached without a device alias")); return -1; } if (!async) - qemuDomainMarkDeviceForRemoval(vm, detach->info); + qemuDomainMarkDeviceForRemoval(vm, hostdev->info); - if (qemuDomainDeleteDevice(vm, detach->info->alias) < 0) { + if (qemuDomainDeleteDevice(vm, hostdev->info->alias) < 0) { if (virDomainObjIsActive(vm)) - virDomainAuditHostdev(vm, detach, "detach", false); + virDomainAuditHostdev(vm, hostdev, "detach", false); goto cleanup; } @@ -5706,7 +5706,7 @@ qemuDomainDetachHostDevice(virQEMUDriverPtr driver, ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveHostDevice(driver, vm, detach); + ret = qemuDomainRemoveHostDevice(driver, vm, hostdev); } cleanup: @@ -5719,24 +5719,25 @@ qemuDomainDetachHostDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainShmemDefPtr dev, - bool async) +qemuDomainDetachPrepShmem(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainShmemDefPtr match, + virDomainShmemDefPtr *detach, + bool async) { int ret = -1; ssize_t idx = -1; virDomainShmemDefPtr shmem = NULL; - if ((idx = virDomainShmemDefFind(vm->def, dev)) < 0) { + if ((idx = virDomainShmemDefFind(vm->def, match)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, _("model '%s' shmem device not present " "in domain configuration"), - virDomainShmemModelTypeToString(dev->model)); + virDomainShmemModelTypeToString(match->model)); return -1; } - shmem = vm->def->shmems[idx]; + *detach = shmem = vm->def->shmems[idx]; switch ((virDomainShmemModel)shmem->model) { case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: @@ -5773,13 +5774,17 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachWatchdog(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainWatchdogDefPtr dev, - bool async) +qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainWatchdogDefPtr match, + virDomainWatchdogDefPtr *detach, + bool async) { int ret = -1; - virDomainWatchdogDefPtr watchdog = vm->def->watchdog; + virDomainWatchdogDefPtr watchdog; + + + *detach = watchdog = vm->def->watchdog; if (!watchdog) { virReportError(VIR_ERR_DEVICE_MISSING, "%s", @@ -5790,9 +5795,9 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, /* While domains can have up to one watchdog, the one supplied by the user * doesn't necessarily match the one domain has. Refuse to detach in such * case. */ - if (!(watchdog->model == dev->model && - watchdog->action == dev->action && - virDomainDeviceInfoAddressIsEqual(&dev->info, &watchdog->info))) { + if (!(watchdog->model == match->model && + watchdog->action == match->action && + virDomainDeviceInfoAddressIsEqual(&match->info, &watchdog->info))) { virReportError(VIR_ERR_DEVICE_MISSING, _("model '%s' watchdog device not present " "in domain configuration"), @@ -5828,40 +5833,41 @@ qemuDomainDetachWatchdog(virQEMUDriverPtr driver, static int -qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainRedirdevDefPtr dev, - bool async) +qemuDomainDetachPrepRedirdev(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRedirdevDefPtr match, + virDomainRedirdevDefPtr *detach, + bool async) { int ret = -1; - virDomainRedirdevDefPtr tmpRedirdevDef; + virDomainRedirdevDefPtr redirdev; ssize_t idx; - if ((idx = virDomainRedirdevDefFind(vm->def, dev)) < 0) { + if ((idx = virDomainRedirdevDefFind(vm->def, match)) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("no matching redirdev was not found")); return -1; } - tmpRedirdevDef = vm->def->redirdevs[idx]; + *detach = redirdev = vm->def->redirdevs[idx]; - if (!tmpRedirdevDef->info.alias) { + if (!redirdev->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("alias not set for redirdev device")); return -1; } if (!async) - qemuDomainMarkDeviceForRemoval(vm, &tmpRedirdevDef->info); + qemuDomainMarkDeviceForRemoval(vm, &redirdev->info); - if (qemuDomainDeleteDevice(vm, tmpRedirdevDef->info.alias) < 0) + if (qemuDomainDeleteDevice(vm, redirdev->info.alias) < 0) goto cleanup; if (async) { ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveRedirdevDevice(driver, vm, tmpRedirdevDef); + ret = qemuDomainRemoveRedirdevDevice(driver, vm, redirdev); } cleanup: @@ -5872,53 +5878,54 @@ qemuDomainDetachRedirdevDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachNetDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) +qemuDomainDetachPrepNet(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainNetDefPtr match, + virDomainNetDefPtr *detach, + bool async) { int detachidx, ret = -1; - virDomainNetDefPtr detach = NULL; + virDomainNetDefPtr net = NULL; - if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0) + if ((detachidx = virDomainNetFindIdx(vm->def, match)) < 0) goto cleanup; - detach = vm->def->nets[detachidx]; + *detach = net = vm->def->nets[detachidx]; - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { + if (qemuIsMultiFunctionDevice(vm->def, &net->info)) { virReportError(VIR_ERR_OPERATION_FAILED, _("cannot hot unplug multifunction PCI device: %s"), - detach->ifname); + net->ifname); goto cleanup; } - if (!detach->info.alias) { - if (qemuAssignDeviceNetAlias(vm->def, detach, -1) < 0) + if (!net->info.alias) { + if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0) goto cleanup; } - if (virDomainNetGetActualBandwidth(detach) && - virNetDevSupportBandwidth(virDomainNetGetActualType(detach)) && - virNetDevBandwidthClear(detach->ifname) < 0) + if (virDomainNetGetActualBandwidth(net) && + virNetDevSupportBandwidth(virDomainNetGetActualType(net)) && + virNetDevBandwidthClear(net->ifname) < 0) VIR_WARN("cannot clear bandwidth setting for device : %s", - detach->ifname); + net->ifname); /* deactivate the tap/macvtap device on the host, which could also * affect the parent device (e.g. macvtap passthrough mode sets * the parent device offline) */ - ignore_value(qemuInterfaceStopDevice(detach)); + ignore_value(qemuInterfaceStopDevice(net)); if (!async) - qemuDomainMarkDeviceForRemoval(vm, &detach->info); + qemuDomainMarkDeviceForRemoval(vm, &net->info); - if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0) { + if (qemuDomainDeleteDevice(vm, net->info.alias) < 0) { if (virDomainObjIsActive(vm)) { /* the audit message has a different format for hostdev network devices */ - if (virDomainNetGetActualType(detach) == VIR_DOMAIN_NET_TYPE_HOSTDEV) - virDomainAuditHostdev(vm, virDomainNetGetActualHostdev(detach), "detach", false); + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) + virDomainAuditHostdev(vm, virDomainNetGetActualHostdev(net), "detach", false); else - virDomainAuditNet(vm, detach, NULL, "detach", false); + virDomainAuditNet(vm, net, NULL, "detach", false); } goto cleanup; } @@ -5927,7 +5934,7 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveNetDevice(driver, vm, detach); + ret = qemuDomainRemoveNetDevice(driver, vm, net); } cleanup: @@ -5996,42 +6003,43 @@ qemuDomainDetachChrDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainRNGDefPtr rng, - bool async) +qemuDomainDetachPrepRNG(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainRNGDefPtr match, + virDomainRNGDefPtr *detach, + bool async) { ssize_t idx; - virDomainRNGDefPtr tmpRNG; + virDomainRNGDefPtr rng; int ret = -1; - if ((idx = virDomainRNGFind(vm->def, rng)) < 0) { + if ((idx = virDomainRNGFind(vm->def, match)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, _("model '%s' RNG device not present " "in domain configuration"), - virDomainRNGBackendTypeToString(rng->model)); + virDomainRNGBackendTypeToString(match->model)); return -1; } - tmpRNG = vm->def->rngs[idx]; + *detach = rng = vm->def->rngs[idx]; - if (!tmpRNG->info.alias) { + if (!rng->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("alias not set for RNG device")); return -1; } if (!async) - qemuDomainMarkDeviceForRemoval(vm, &tmpRNG->info); + qemuDomainMarkDeviceForRemoval(vm, &rng->info); - if (qemuDomainDeleteDevice(vm, tmpRNG->info.alias) < 0) + if (qemuDomainDeleteDevice(vm, rng->info.alias) < 0) goto cleanup; if (async) { ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveRNGDevice(driver, vm, tmpRNG); + ret = qemuDomainRemoveRNGDevice(driver, vm, rng); } cleanup: @@ -6042,26 +6050,27 @@ qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainMemoryDefPtr memdef, - bool async) +qemuDomainDetachPrepMemory(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainMemoryDefPtr match, + virDomainMemoryDefPtr *detach, + bool async) { virDomainMemoryDefPtr mem; int idx; int ret = -1; - qemuDomainMemoryDeviceAlignSize(vm->def, memdef); + qemuDomainMemoryDeviceAlignSize(vm->def, match); - if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) { + if ((idx = virDomainMemoryFindByDef(vm->def, match)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, _("model '%s' memory device not present " "in the domain configuration"), - virDomainMemoryModelTypeToString(memdef->model)); + virDomainMemoryModelTypeToString(match->model)); return -1; } - mem = vm->def->mems[idx]; + *detach = mem = vm->def->mems[idx]; if (!mem->info.alias) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -6090,20 +6099,21 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver, static int -qemuDomainDetachInputDevice(virDomainObjPtr vm, - virDomainInputDefPtr def, - bool async) +qemuDomainDetachPrepInput(virDomainObjPtr vm, + virDomainInputDefPtr match, + virDomainInputDefPtr *detach, + bool async) { virDomainInputDefPtr input; int ret = -1; int idx; - if ((idx = virDomainInputDefFind(vm->def, def)) < 0) { + if ((idx = virDomainInputDefFind(vm->def, match)) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("matching input device not found")); return -1; } - input = vm->def->inputs[idx]; + *detach = input = vm->def->inputs[idx]; switch ((virDomainInputBus) input->bus) { case VIR_DOMAIN_INPUT_BUS_PS2: @@ -6141,16 +6151,18 @@ qemuDomainDetachInputDevice(virDomainObjPtr vm, static int -qemuDomainDetachVsockDevice(virDomainObjPtr vm, - virDomainVsockDefPtr dev, - bool async) +qemuDomainDetachPrepVsock(virDomainObjPtr vm, + virDomainVsockDefPtr match, + virDomainVsockDefPtr *detach, + bool async) { - virDomainVsockDefPtr vsock = vm->def->vsock; + virDomainVsockDefPtr vsock; int ret = -1; + *detach = vsock = vm->def->vsock; if (!vsock || - !virDomainVsockDefEquals(dev, vsock)) { + !virDomainVsockDefEquals(match, vsock)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("matching vsock device not found")); return -1; @@ -6206,6 +6218,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, virQEMUDriverPtr driver, bool async) { + virDomainDeviceDef detach = { .type = match->type }; int ret = -1; switch ((virDomainDeviceType)match->type) { @@ -6228,38 +6241,70 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, * assure it is okay to detach the device. */ case VIR_DOMAIN_DEVICE_DISK: - ret = qemuDomainDetachDeviceDiskLive(driver, vm, match, async); + if (qemuDomainDetachPrepDisk(driver, vm, match->data.disk, + &detach.data.disk, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainDetachControllerDevice(driver, vm, match, async); + if (qemuDomainDetachPrepController(driver, vm, match->data.controller, + &detach.data.controller, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_NET: - ret = qemuDomainDetachNetDevice(driver, vm, match, async); + if (qemuDomainDetachPrepNet(driver, vm, match->data.net, + &detach.data.net, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_HOSTDEV: - ret = qemuDomainDetachHostDevice(driver, vm, match, async); + if (qemuDomainDetachPrepHostdev(driver, vm, match->data.hostdev, + &detach.data.hostdev, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_RNG: - ret = qemuDomainDetachRNGDevice(driver, vm, match->data.rng, async); + if (qemuDomainDetachPrepRNG(driver, vm, match->data.rng, + &detach.data.rng, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_MEMORY: - ret = qemuDomainDetachMemoryDevice(driver, vm, match->data.memory, async); + if (qemuDomainDetachPrepMemory(driver, vm, match->data.memory, + &detach.data.memory, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_SHMEM: - ret = qemuDomainDetachShmemDevice(driver, vm, match->data.shmem, async); + if (qemuDomainDetachPrepShmem(driver, vm, match->data.shmem, + &detach.data.shmem, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_WATCHDOG: - ret = qemuDomainDetachWatchdog(driver, vm, match->data.watchdog, async); + if (qemuDomainDetachPrepWatchdog(driver, vm, match->data.watchdog, + &detach.data.watchdog, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_INPUT: - ret = qemuDomainDetachInputDevice(vm, match->data.input, async); + if (qemuDomainDetachPrepInput(vm, match->data.input, + &detach.data.input, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_REDIRDEV: - ret = qemuDomainDetachRedirdevDevice(driver, vm, match->data.redirdev, async); + if (qemuDomainDetachPrepRedirdev(driver, vm, match->data.redirdev, + &detach.data.redirdev, async) < 0) { + return -1; + } break; - case VIR_DOMAIN_DEVICE_VSOCK: - ret = qemuDomainDetachVsockDevice(vm, match->data.vsock, async); + if (qemuDomainDetachPrepVsock(vm, match->data.vsock, + &detach.data.vsock, async) < 0) { + return -1; + } break; case VIR_DOMAIN_DEVICE_FS: @@ -6281,6 +6326,8 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, return -1; } + ret = 0; + return ret; } -- 2.20.1

On 3/26/19 8:40 AM, Peter Krempa wrote: Yeah, valid point. I just sometimes grow weary of changing a line in one patch just to change the same line again another. But I guess I do that enough anyway, so one more time shouldn't bother me :-P I'll be sure to inflate my patch count as much as reasonably possible next time! I'm not a fan of *not* putting curly braces here :-) Good point. I'm guessing it's because many others of these functions also started out in qemu_driver.c (and at a time when were less consistent with function naming) and were moved to this file with no name change. A followup patch to make the names all consistent might be nice... Yes sir!

On Mon, Mar 25, 2019 at 13:24:32 -0400, Laine Stump wrote: This is actually misleading. The guest can ask for removal of a PCI device. It's a bug in libvirt that the backend removal functions along with deleting of the definition is not implemented. I'd just state that they currently don't have detach handlers implemented. ACK regardless as this requires more fixing out of the scope.

On 3/26/19 12:58 PM, Ján Tomko wrote: Whew! This bothered me for a minute, since I'd already pushed everything, but when I looked at what was pushed, I realized that Peter requested different wording for the comment, and in the process of doing that, I had accidentally fixed the indentation. Thanks for the attention to detail though!

On Mon, Mar 25, 2019 at 13:24:33 -0400, Laine Stump wrote: This probably should also say "detach" as the rest does. ACK

Now that all the qemuDomainDetachPrep*() functions look nearly identical at the end, we can put one copy of that identical code in qemuDomainDetachDeviceLive() at the point after the individual prep functions have been called, and remove the duplicated code from all the prep functions. The code to locate the target "detach" device based on the "match" device remains, as do all device-type-specific validations. Unfortunately there are a few things going on at once in this patch, which makes it a bit more difficult to follow than the others; it was just impossible to do the changes in stages and still have a buildable/testable tree at each step. The other changes of note: * The individual prep functions no longer need their driver or async args, so those are removed, as are the local "ret" variables, since in all cases the functions just directly return -1 or 0. * Some of the prep functions were checking for a valid alias and/or for attempts to detach a multifunction PCI device, but not all. In fact, both checks are valid (or at least harmless) for *all* device types, so they are removed from the prep functions, and done a single time in the common function. (any attempts to *create* an alias when there isn't one has been removed, since that is doomed to failure anyway; the only way the device wouldn't have an alias is if 1) the domain was created by calling virsh qemu-attach to attach an existing qemu process to libvirt, and 2) the qemu command that started said process used "old style" arguments for creating devices that didn't have any device ids. Even if we constructed a device id for one of these devices, qemu wouldn't recognize it in the device_del command anyway, so we may as well fail earlier with "device missing alias" rather than failing later with "couldn't delete device net0".) * Only one type of device has shutdown code that must not be called until after *all* validation of the device is done (including checking for multifunction PCI and valid alias, which is done in the toplevel common code). For this reason, the Net function has been split in two, with the 2nd half (qemuDomainDetachShutdownNet()) called from the common function, right before sending the delete command to qemu. Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- Change from V1: * netdev shutdown is *not* moved to RemoveNet function - yet! That is a good idea, but it is being done in a separate patch (14/14) in order to make it easy to revert if we find it causes a problem. * src/qemu/qemu_hotplug.c | 481 ++++++++++++---------------------------- 1 file changed, 142 insertions(+), 339 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e9d6c8622b..c86d1fb4eb 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5208,7 +5208,7 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, } -static void ATTRIBUTE_UNUSED +static void qemuDomainRemoveAuditDevice(virDomainObjPtr vm, virDomainDeviceDefPtr detach, bool success) @@ -5454,15 +5454,12 @@ qemuFindDisk(virDomainDefPtr def, const char *dst) } static int -qemuDomainDetachPrepDisk(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepDisk(virDomainObjPtr vm, virDomainDiskDefPtr match, - virDomainDiskDefPtr *detach, - bool async) + virDomainDiskDefPtr *detach) { virDomainDiskDefPtr disk; int idx; - int ret = -1; if ((idx = qemuFindDisk(vm->def, match->dst)) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, @@ -5514,34 +5511,7 @@ qemuDomainDetachPrepDisk(virQEMUDriverPtr driver, if (qemuDomainDiskBlockJobIsActive(disk)) return -1; - if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && - qemuIsMultiFunctionDevice(vm->def, &disk->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %s"), - disk->dst); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &disk->info); - - if (qemuDomainDeleteDevice(vm, disk->info.alias) < 0) { - if (virDomainObjIsActive(vm)) - virDomainAuditDisk(vm, disk->src, NULL, "detach", false); - goto cleanup; - } - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveDiskDevice(driver, vm, disk); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } @@ -5606,13 +5576,11 @@ static bool qemuDomainControllerIsBusy(virDomainObjPtr vm, } static int -qemuDomainDetachPrepController(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepController(virDomainObjPtr vm, virDomainControllerDefPtr match, - virDomainControllerDefPtr *detach, - bool async) + virDomainControllerDefPtr *detach) { - int idx, ret = -1; + int idx; virDomainControllerDefPtr controller = NULL; if (match->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { @@ -5627,50 +5595,26 @@ qemuDomainDetachPrepController(virQEMUDriverPtr driver, _("controller %s:%d not found"), virDomainControllerTypeToString(match->type), match->idx); - goto cleanup; + return -1; } *detach = controller = vm->def->controllers[idx]; - if (qemuIsMultiFunctionDevice(vm->def, &controller->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("cannot hot unplug multifunction PCI device")); - goto cleanup; - } - if (qemuDomainControllerIsBusy(vm, controller)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("device cannot be detached: device is busy")); - goto cleanup; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &controller->info); - - if (qemuDomainDeleteDevice(vm, controller->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveControllerDevice(driver, vm, controller); + return -1; } - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } /* search for a hostdev matching dev and detach it */ static int -qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepHostdev(virDomainObjPtr vm, virDomainHostdevDefPtr match, - virDomainHostdevDefPtr *detach, - bool async) + virDomainHostdevDefPtr *detach) { virDomainHostdevSubsysPtr subsys = &match->source.subsys; virDomainHostdevSubsysUSBPtr usbsrc = &subsys->u.usb; @@ -5679,7 +5623,6 @@ qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver, virDomainHostdevSubsysMediatedDevPtr mdevsrc = &subsys->u.mdev; virDomainHostdevDefPtr hostdev = NULL; int idx; - int ret = -1; if (match->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -5742,54 +5685,15 @@ qemuDomainDetachPrepHostdev(virQEMUDriverPtr driver, return -1; } - if (qemuIsMultiFunctionDevice(vm->def, hostdev->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device with guest address: " - "%.4x:%.2x:%.2x.%.1x"), - hostdev->info->addr.pci.domain, hostdev->info->addr.pci.bus, - hostdev->info->addr.pci.slot, hostdev->info->addr.pci.function); - return -1; - } - - if (!hostdev->info->alias) { - virReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("device cannot be detached without a device alias")); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, hostdev->info); - - if (qemuDomainDeleteDevice(vm, hostdev->info->alias) < 0) { - if (virDomainObjIsActive(vm)) - virDomainAuditHostdev(vm, hostdev, "detach", false); - goto cleanup; - } - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveHostDevice(driver, vm, hostdev); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - - return ret; - + return 0; } static int -qemuDomainDetachPrepShmem(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepShmem(virDomainObjPtr vm, virDomainShmemDefPtr match, - virDomainShmemDefPtr *detach, - bool async) + virDomainShmemDefPtr *detach) { - int ret = -1; ssize_t idx = -1; virDomainShmemDefPtr shmem = NULL; @@ -5817,34 +5721,15 @@ qemuDomainDetachPrepShmem(virQEMUDriverPtr driver, return -1; } - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &shmem->info); - - if (qemuDomainDeleteDevice(vm, shmem->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveShmemDevice(driver, vm, shmem); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int -qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepWatchdog(virDomainObjPtr vm, virDomainWatchdogDefPtr match, - virDomainWatchdogDefPtr *detach, - bool async) + virDomainWatchdogDefPtr *detach) { - int ret = -1; virDomainWatchdogDefPtr watchdog; @@ -5876,34 +5761,15 @@ qemuDomainDetachPrepWatchdog(virQEMUDriverPtr driver, return -1; } - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &watchdog->info); - - if (qemuDomainDeleteDevice(vm, watchdog->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveWatchdog(driver, vm, watchdog); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int -qemuDomainDetachPrepRedirdev(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepRedirdev(virDomainObjPtr vm, virDomainRedirdevDefPtr match, - virDomainRedirdevDefPtr *detach, - bool async) + virDomainRedirdevDefPtr *detach) { - int ret = -1; virDomainRedirdevDefPtr redirdev; ssize_t idx; @@ -5915,59 +5781,41 @@ qemuDomainDetachPrepRedirdev(virQEMUDriverPtr driver, *detach = redirdev = vm->def->redirdevs[idx]; - if (!redirdev->info.alias) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("alias not set for redirdev device")); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &redirdev->info); - - if (qemuDomainDeleteDevice(vm, redirdev->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveRedirdevDevice(driver, vm, redirdev); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int -qemuDomainDetachPrepNet(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepNet(virDomainObjPtr vm, virDomainNetDefPtr match, - virDomainNetDefPtr *detach, - bool async) + virDomainNetDefPtr *detach) { - int detachidx, ret = -1; + int detachidx; virDomainNetDefPtr net = NULL; if ((detachidx = virDomainNetFindIdx(vm->def, match)) < 0) - goto cleanup; + return -1; *detach = net = vm->def->nets[detachidx]; - if (qemuIsMultiFunctionDevice(vm->def, &net->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %s"), - net->ifname); - goto cleanup; - } + return 0; +} - if (!net->info.alias) { - if (qemuAssignDeviceNetAlias(vm->def, net, -1) < 0) - goto cleanup; - } +static void +qemuDomainDetachShutdownNet(virDomainNetDefPtr net) +{ +/* + * These operations are in a separate function from + * qemuDomainDetachPrepNet() because they can't be done until after + * we've validated that this device really can be removed - in + * particular we need to check for multifunction PCI devices and + * presence of a device alias, which isn't done until *after* the + * return from qemuDomainDetachPrepNet(). Since we've already passed + * the "point of no return", we ignore any errors, and trudge ahead + * with shutting down and detaching the device even if there is an + * error in one of these functions. + */ if (virDomainNetGetActualBandwidth(net) && virNetDevSupportBandwidth(virDomainNetGetActualType(net)) && virNetDevBandwidthClear(net->ifname) < 0) @@ -5979,35 +5827,8 @@ qemuDomainDetachPrepNet(virQEMUDriverPtr driver, * the parent device offline) */ ignore_value(qemuInterfaceStopDevice(net)); - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &net->info); - - if (qemuDomainDeleteDevice(vm, net->info.alias) < 0) { - if (virDomainObjIsActive(vm)) { - /* the audit message has a different format for hostdev network devices */ - if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) - virDomainAuditHostdev(vm, virDomainNetGetActualHostdev(net), "detach", false); - else - virDomainAuditNet(vm, net, NULL, "detach", false); - } - goto cleanup; - } - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveNetDevice(driver, vm, net); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; } - static int qemuDomainDetachDeviceChr(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -6067,15 +5888,12 @@ qemuDomainDetachDeviceChr(virQEMUDriverPtr driver, static int -qemuDomainDetachPrepRNG(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepRNG(virDomainObjPtr vm, virDomainRNGDefPtr match, - virDomainRNGDefPtr *detach, - bool async) + virDomainRNGDefPtr *detach) { ssize_t idx; virDomainRNGDefPtr rng; - int ret = -1; if ((idx = virDomainRNGFind(vm->def, match)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, @@ -6087,42 +5905,17 @@ qemuDomainDetachPrepRNG(virQEMUDriverPtr driver, *detach = rng = vm->def->rngs[idx]; - if (!rng->info.alias) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("alias not set for RNG device")); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &rng->info); - - if (qemuDomainDeleteDevice(vm, rng->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveRNGDevice(driver, vm, rng); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int -qemuDomainDetachPrepMemory(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainDetachPrepMemory(virDomainObjPtr vm, virDomainMemoryDefPtr match, - virDomainMemoryDefPtr *detach, - bool async) + virDomainMemoryDefPtr *detach) { virDomainMemoryDefPtr mem; int idx; - int ret = -1; qemuDomainMemoryDeviceAlignSize(vm->def, match); @@ -6136,40 +5929,16 @@ qemuDomainDetachPrepMemory(virQEMUDriverPtr driver, *detach = mem = vm->def->mems[idx]; - if (!mem->info.alias) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("alias for the memory device was not found")); - return -1; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &mem->info); - - if (qemuDomainDeleteDevice(vm, mem->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveMemoryDevice(driver, vm, mem); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int qemuDomainDetachPrepInput(virDomainObjPtr vm, virDomainInputDefPtr match, - virDomainInputDefPtr *detach, - bool async) + virDomainInputDefPtr *detach) { virDomainInputDefPtr input; - int ret = -1; int idx; if ((idx = virDomainInputDefFind(vm->def, match)) < 0) { @@ -6194,35 +5963,16 @@ qemuDomainDetachPrepInput(virDomainObjPtr vm, break; } - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &input->info); - - if (qemuDomainDeleteDevice(vm, input->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveInputDevice(vm, input); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } static int qemuDomainDetachPrepVsock(virDomainObjPtr vm, virDomainVsockDefPtr match, - virDomainVsockDefPtr *detach, - bool async) + virDomainVsockDefPtr *detach) { virDomainVsockDefPtr vsock; - int ret = -1; - *detach = vsock = vm->def->vsock; if (!vsock || @@ -6232,23 +5982,7 @@ qemuDomainDetachPrepVsock(virDomainObjPtr vm, return -1; } - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &vsock->info); - - if (qemuDomainDeleteDevice(vm, vsock->info.alias) < 0) - goto cleanup; - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveVsockDevice(vm, vsock); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; + return 0; } @@ -6283,6 +6017,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, bool async) { virDomainDeviceDef detach = { .type = match->type }; + virDomainDeviceInfoPtr info = NULL; int ret = -1; switch ((virDomainDeviceType)match->type) { @@ -6305,68 +6040,68 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, * assure it is okay to detach the device. */ case VIR_DOMAIN_DEVICE_DISK: - if (qemuDomainDetachPrepDisk(driver, vm, match->data.disk, - &detach.data.disk, async) < 0) { + if (qemuDomainDetachPrepDisk(vm, match->data.disk, + &detach.data.disk) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_CONTROLLER: - if (qemuDomainDetachPrepController(driver, vm, match->data.controller, - &detach.data.controller, async) < 0) { + if (qemuDomainDetachPrepController(vm, match->data.controller, + &detach.data.controller) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_NET: - if (qemuDomainDetachPrepNet(driver, vm, match->data.net, - &detach.data.net, async) < 0) { + if (qemuDomainDetachPrepNet(vm, match->data.net, + &detach.data.net) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_HOSTDEV: - if (qemuDomainDetachPrepHostdev(driver, vm, match->data.hostdev, - &detach.data.hostdev, async) < 0) { + if (qemuDomainDetachPrepHostdev(vm, match->data.hostdev, + &detach.data.hostdev) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_RNG: - if (qemuDomainDetachPrepRNG(driver, vm, match->data.rng, - &detach.data.rng, async) < 0) { + if (qemuDomainDetachPrepRNG(vm, match->data.rng, + &detach.data.rng) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_MEMORY: - if (qemuDomainDetachPrepMemory(driver, vm, match->data.memory, - &detach.data.memory, async) < 0) { + if (qemuDomainDetachPrepMemory(vm, match->data.memory, + &detach.data.memory) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_SHMEM: - if (qemuDomainDetachPrepShmem(driver, vm, match->data.shmem, - &detach.data.shmem, async) < 0) { + if (qemuDomainDetachPrepShmem(vm, match->data.shmem, + &detach.data.shmem) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_WATCHDOG: - if (qemuDomainDetachPrepWatchdog(driver, vm, match->data.watchdog, - &detach.data.watchdog, async) < 0) { + if (qemuDomainDetachPrepWatchdog(vm, match->data.watchdog, + &detach.data.watchdog) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_INPUT: if (qemuDomainDetachPrepInput(vm, match->data.input, - &detach.data.input, async) < 0) { + &detach.data.input) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_REDIRDEV: - if (qemuDomainDetachPrepRedirdev(driver, vm, match->data.redirdev, - &detach.data.redirdev, async) < 0) { + if (qemuDomainDetachPrepRedirdev(vm, match->data.redirdev, + &detach.data.redirdev) < 0) { return -1; } break; case VIR_DOMAIN_DEVICE_VSOCK: if (qemuDomainDetachPrepVsock(vm, match->data.vsock, - &detach.data.vsock, async) < 0) { + &detach.data.vsock) < 0) { return -1; } break; @@ -6390,7 +6125,75 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, return -1; } - ret = 0; + /* "detach" now points to the actual device we want to detach */ + + if (!(info = virDomainDeviceGetInfo(&detach))) { + /* + * This should never happen, since all of the device types in + * the switch cases that end with a "break" instead of a + * return have a virDeviceInfo in them. + */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("device of type '%s' has no device info"), + virDomainDeviceTypeToString(detach.type)); + return -1; + } + + + /* Make generic validation checks common to all device types */ + + if (!info->alias) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot detach %s device with no alias"), + virDomainDeviceTypeToString(detach.type)); + return -1; + } + + if (qemuIsMultiFunctionDevice(vm->def, info)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("cannot hot unplug %s device with multifunction PCI guest address: " + "%.4x:%.2x:%.2x.%.1x"), + virDomainDeviceTypeToString(detach.type), + info->addr.pci.domain, info->addr.pci.bus, + info->addr.pci.slot, info->addr.pci.function); + return -1; + } + + + /* + * Do any device-specific shutdown that should be + * done after all validation checks, but before issuing the qemu + * command to delete the device. For now, the only type of device + * that has such shutdown needs is the net device. + */ + if (detach.type == VIR_DOMAIN_DEVICE_NET) + qemuDomainDetachShutdownNet(detach.data.net); + + + /* + * Issue the qemu monitor command to delete the device (based on + * its alias), and optionally wait a short time in case the + * DEVICE_DELETED event arrives from qemu right away. + */ + if (!async) + qemuDomainMarkDeviceForRemoval(vm, info); + + if (qemuDomainDeleteDevice(vm, info->alias) < 0) { + if (virDomainObjIsActive(vm)) + qemuDomainRemoveAuditDevice(vm, &detach, false); + goto cleanup; + } + + if (async) { + ret = 0; + } else { + if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) + ret = qemuDomainRemoveDevice(driver, vm, &detach); + } + + cleanup: + if (!async) + qemuDomainResetDeviceRemoval(vm); return ret; } -- 2.20.1

The VIR_DOMAIN_EVENT_ID_DEVICE_REMOVED event is sent after qemu has responded to a device_del command with a DEVICE_DELETED event. Before queuing the event, *some* of the final teardown of the device's trappings in libvirt is done, but not *all* of it. As a result, an application may receive and process the DEVICE_REMOVED event before libvirt has really finished with it. Usually this doesn't cause a problem, but it can - in the case of the bug report referenced below, vdsm is assigning a PCI device to a guest with managed='no', using livirt's virNodeDeviceDetachFlags() and virNodeDeviceReAttach() APIs. Immediately after receiving a DEVICE_REMOVED event from libvirt signalling that the device had been successfully unplugged, vdsm would cal virNodeDeviceReAttach() to unbind the device from vfio-pci and rebind it to the host driverm but because the event was received before libvirt had completely finished processing the removal, that device was still on the "activeDevs" list, and so virNodeDeviceReAttach() failed. Experimentation with additional debug logs proved that libvirt would always end up dispatching the DEVICE_REMOVED event before it had removed the device from activeDevs (with a *much* greater difference with managed='yes', since in that case the re-binding of the device occurred after queuing the device). Although the case of hostdev devices is the most extreme (since there is so much involved in tearing down the device), *all* device types suffer from the same problem - the DEVICE_REMOVED event is queued very early in the qemuDomainRemove*Device() function for all of them, resulting in a possibility of any application receiving the event before libvirt has really finished with the device. The solution is to save the device's alias (which is the only piece of info from the device object that is needed for the event) at the beginning of processing the device removal, and then queue the event as a final act before returning. Since all of the qemuDomainRemove*Device() functions (except qemuDomainRemoveChrDevice()) are now called exclusively from qemuDomainRemoveDevice() (which selects which of the subordinates to call in a switch statement based on the type of device), the shortest route to a solution is to doing the saving of alias, and later queueing of the event, in the higher level qemuDomainRemoveDevice(), and just completely remove the event-related code from all the subordinate functions. The single exception to this, as mentioned before, is qemuDomainRemoveChrDevice(), which is still called from somewhere other than qemuDomainRemoveDevice() (and has a separate arg used to trigger different behavior when the chr device has targetType == GUESTFWD), so it must keep its original behavior intact, and must be treated differently by qemuDomainRemoveDevice() (similar to the way that qemuDomainDetachDeviceLive() treats chr and lease devices differently from all the others). Resolves: https://bugzilla.redhat.com/1658198 Signed-off-by: Laine Stump <laine(a)laine.org&gt; --- Change from V1: * reworded some comments based on review * don't error out if the device has no DeviceInfo, instead just don't sent the DEVICE_REMOVED event. * Set DeviceInfoPtr to NULL after we've retrieved the alias from it. src/qemu/qemu_hotplug.c | 145 +++++++++++++++++++--------------------- 1 file changed, 69 insertions(+), 76 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c86d1fb4eb..78890f4bbe 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4501,7 +4501,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, { qemuHotplugDiskSourceDataPtr diskbackend = NULL; virDomainDeviceDef dev; - virObjectEventPtr event; size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; @@ -4529,9 +4528,6 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virDomainAuditDisk(vm, disk->src, NULL, "detach", true); - event = virDomainEventDeviceRemovedNewFromObj(vm, disk->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); - qemuDomainReleaseDeviceAddress(vm, &disk->info, virDomainDiskGetSource(disk)); /* tear down disk security access */ @@ -4555,19 +4551,14 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, static int -qemuDomainRemoveControllerDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainRemoveControllerDevice(virDomainObjPtr vm, virDomainControllerDefPtr controller) { - virObjectEventPtr event; size_t i; VIR_DEBUG("Removing controller %s from domain %p %s", controller->info.alias, vm, vm->def->name); - event = virDomainEventDeviceRemovedNewFromObj(vm, controller->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); - for (i = 0; i < vm->def->ncontrollers; i++) { if (vm->def->controllers[i] == controller) { virDomainControllerRemove(vm->def, i); @@ -4589,7 +4580,6 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def); unsigned long long newmem = oldmem - mem->size; - virObjectEventPtr event; char *backendAlias = NULL; int rc; int idx; @@ -4611,9 +4601,6 @@ qemuDomainRemoveMemoryDevice(virQEMUDriverPtr driver, if (rc < 0) return -1; - event = virDomainEventDeviceRemovedNewFromObj(vm, mem->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); - if ((idx = virDomainMemoryFindByDef(vm->def, mem)) >= 0) virDomainMemoryRemove(vm->def, idx); @@ -4693,7 +4680,6 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainNetDefPtr net = NULL; - virObjectEventPtr event; size_t i; int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -4737,9 +4723,6 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, goto cleanup; } - event = virDomainEventDeviceRemovedNewFromObj(vm, hostdev->info->alias); - virObjectEventStateQueue(driver->domainEventState, event); - if (hostdev->parent.type == VIR_DOMAIN_DEVICE_NET) { net = hostdev->parent.data.net; @@ -4818,7 +4801,6 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; - virObjectEventPtr event; char *hostnet_name = NULL; char *charDevAlias = NULL; size_t i; @@ -4863,9 +4845,6 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, virDomainAuditNet(vm, net, NULL, "detach", true); - event = virDomainEventDeviceRemovedNewFromObj(vm, net->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); - for (i = 0; i < vm->def->nnets; i++) { if (vm->def->nets[i] == net) { virDomainNetRemove(vm->def, i); @@ -4948,11 +4927,20 @@ qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, if (qemuDomainNamespaceTeardownChardev(vm, chr) < 0) VIR_WARN("Unable to remove chr device from /dev"); + qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); + qemuDomainChrRemove(vm->def, chr); + + /* NB: all other qemuDomainRemove*Device() functions omit the + * sending of the DEVICE_REMOVED event, because they are *only* + * called from qemuDomainRemoveDevice(), and that function sends + * the DEVICE_REMOVED event for them, this function is different - + * it is called from other places than just + * qemuDomainRemoveDevice(), so it must send the DEVICE_REMOVED + * event itself. + */ event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias); virObjectEventStateQueue(driver->domainEventState, event); - qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); - qemuDomainChrRemove(vm->def, chr); virDomainChrDefFree(chr); ret = 0; @@ -4967,7 +4955,6 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainRNGDefPtr rng) { - virObjectEventPtr event; char *charAlias = NULL; char *objAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -5016,9 +5003,6 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, if (qemuDomainNamespaceTeardownRNG(vm, rng) < 0) VIR_WARN("Unable to remove RNG device from /dev"); - event = virDomainEventDeviceRemovedNewFromObj(vm, rng->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); - if ((idx = virDomainRNGFind(vm->def, rng)) >= 0) virDomainRNGRemove(vm->def, idx); qemuDomainReleaseDeviceAddress(vm, &rng->info, NULL); @@ -5043,7 +5027,6 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, 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); @@ -5071,9 +5054,6 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, if (rc < 0) goto cleanup; - event = virDomainEventDeviceRemovedNewFromObj(vm, shmem->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); - if ((idx = virDomainShmemDefFind(vm->def, shmem)) >= 0) virDomainShmemDefRemove(vm->def, idx); qemuDomainReleaseDeviceAddress(vm, &shmem->info, NULL); @@ -5089,17 +5069,12 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver, static int -qemuDomainRemoveWatchdog(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainRemoveWatchdog(virDomainObjPtr vm, virDomainWatchdogDefPtr watchdog) { - virObjectEventPtr event = NULL; - VIR_DEBUG("Removing watchdog %s from domain %p %s", watchdog->info.alias, vm, vm->def->name); - event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL); virDomainWatchdogDefFree(vm->def->watchdog); vm->def->watchdog = NULL; @@ -5111,16 +5086,11 @@ static int qemuDomainRemoveInputDevice(virDomainObjPtr vm, virDomainInputDefPtr dev) { - qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverPtr driver = priv->driver; - virObjectEventPtr event = NULL; size_t i; VIR_DEBUG("Removing input device %s from domain %p %s", dev->info.alias, vm, vm->def->name); - event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); for (i = 0; i < vm->def->ninputs; i++) { if (vm->def->inputs[i] == dev) break; @@ -5145,15 +5115,9 @@ static int qemuDomainRemoveVsockDevice(virDomainObjPtr vm, virDomainVsockDefPtr dev) { - qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverPtr driver = priv->driver; - virObjectEventPtr event = NULL; - VIR_DEBUG("Removing vsock device %s from domain %p %s", dev->info.alias, vm, vm->def->name); - event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); qemuDomainReleaseDeviceAddress(vm, &dev->info, NULL); virDomainVsockDefFree(vm->def->vsock); vm->def->vsock = NULL; @@ -5167,7 +5131,6 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, virDomainRedirdevDefPtr dev) { qemuDomainObjPrivatePtr priv = vm->privateData; - virObjectEventPtr event; char *charAlias = NULL; ssize_t idx; int ret = -1; @@ -5192,9 +5155,6 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, virDomainAuditRedirdev(vm, dev, "detach", true); - event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias); - virObjectEventStateQueue(driver->domainEventState, event); - if ((idx = virDomainRedirdevDefFind(vm->def, dev)) >= 0) virDomainRedirdevDefRemove(vm->def, idx); qemuDomainReleaseDeviceAddress(vm, &dev->info, NULL); @@ -5277,50 +5237,79 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { - int ret = -1; + virDomainDeviceInfoPtr info; + virObjectEventPtr event; + VIR_AUTOFREE(char *)alias = NULL; + + /* + * save the alias to use when sending a DEVICE_REMOVED event after + * all other teardown is complete + */ + if ((info = virDomainDeviceGetInfo(dev)) + && VIR_STRDUP(alias, info->alias) < 0) { + return -1; + } + info = NULL; /* to prevent accidental use later */ + switch ((virDomainDeviceType)dev->type) { + case VIR_DOMAIN_DEVICE_CHR: + /* We must return directly after calling + * qemuDomainRemoveChrDevice because it is called directly + * from other places, so it must be completely self-contained + * and can't take advantage of any common code at the end of + * qemuDomainRemoveDevice(). + */ + return qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true); + + /* + * all of the following qemuDomainRemove*Device() functions + * are (and must be) only called from this function, so any + * code that is common to them all can be pulled out and put + * into this function. + */ case VIR_DOMAIN_DEVICE_DISK: - ret = qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk); + if (qemuDomainRemoveDiskDevice(driver, vm, dev->data.disk) < 0) + return -1; break; case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainRemoveControllerDevice(driver, vm, dev->data.controller); + if (qemuDomainRemoveControllerDevice(vm, dev->data.controller) < 0) + return -1; break; case VIR_DOMAIN_DEVICE_NET: - ret = qemuDomainRemoveNetDevice(driver, vm, dev->data.net); + if (qemuDomainRemoveNetDevice(driver, vm, dev->data.net) < 0) + return -1; break; case VIR_DOMAIN_DEVICE_HOSTDEV: - ret = qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev); - break; - - case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true); + if (qemuDomainRemoveHostDevice(driver, vm, dev->data.hostdev) < 0) + return -1; break; case VIR_DOMAIN_DEVICE_RNG: - ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); + if (qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng) < 0) + return -1; break; - case VIR_DOMAIN_DEVICE_MEMORY: - ret = qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory); + if (qemuDomainRemoveMemoryDevice(driver, vm, dev->data.memory) < 0) + return -1; break; - case VIR_DOMAIN_DEVICE_SHMEM: - ret = qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem); + if (qemuDomainRemoveShmemDevice(driver, vm, dev->data.shmem) < 0) + return -1; break; - case VIR_DOMAIN_DEVICE_INPUT: - ret = qemuDomainRemoveInputDevice(vm, dev->data.input); + if (qemuDomainRemoveInputDevice(vm, dev->data.input) < 0) + return -1; break; - case VIR_DOMAIN_DEVICE_REDIRDEV: - ret = qemuDomainRemoveRedirdevDevice(driver, vm, dev->data.redirdev); + if (qemuDomainRemoveRedirdevDevice(driver, vm, dev->data.redirdev) < 0) + return -1; break; - case VIR_DOMAIN_DEVICE_WATCHDOG: - ret = qemuDomainRemoveWatchdog(driver, vm, dev->data.watchdog); + if (qemuDomainRemoveWatchdog(vm, dev->data.watchdog) < 0) + return -1; break; - case VIR_DOMAIN_DEVICE_VSOCK: - ret = qemuDomainRemoveVsockDevice(vm, dev->data.vsock); + if (qemuDomainRemoveVsockDevice(vm, dev->data.vsock) < 0) + return -1; break; case VIR_DOMAIN_DEVICE_NONE: @@ -5342,7 +5331,11 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainDeviceTypeToString(dev->type)); break; } - return ret; + + event = virDomainEventDeviceRemovedNewFromObj(vm, alias); + virObjectEventStateQueue(driver->domainEventState, event); + + return 0; } -- 2.20.1