[libvirt] [PATCH v2 00/14] qemu_hotplug: refactor device detach functions to fix one serious and other minor bugs
V1: https://www.redhat.com/archives/libvir-list/2019-March/msg01455.html I pushed patches 01-12, since they were acked, and pushing them leaves the code in no worse state than it was before (all tests still pass). There are 4 new patches in V2, created as a result of Peter asking for patches to be split, or for further refactoring. Still to be done: Peter pointed out that qemuDomainUpdateDeviceLlist() really shouldn't be called until after qemu has sent the DEVICE_DELETED event. That is on my list of followups, but the patches here are still a valid improvement even without that (After his comment, I noticed that we are also calling virDomainSaveStatus too soon - it also shouldn't be called until after DEVICE_DELETED). Laine Stump (14): qemu_hotplug: move qemuDomainDetachDeviceLive() to qemu_hotplug.c qemu_hotplug: remove extra function in middle of DetachController call chain qemu_hotplug: pull qemuDomainUpdateDeviceList out of qemuDomainDetachDeviceLive test: replace calls to individual detach functions with one call to main detach qemu_hotplug: make Detach functions called only from qemu_hotplug.c static qemu_hotplug: rename dev to match in qemuDomainDetachDeviceLive qemu_hotplug: separate Chr|Lease from other devices in DetachDevice switch qemu_hotplug: standardize the names/args/calling of qemuDomainDetach*() qemu_hotplug: rename Chr and Lease Detach functions qemu_hotplug: new function qemuDomainRemoveAuditDevice() qemu_hotplug: audit *all* auditable device types in qemuDomainRemoveAuditDevice qemu_hotplug: consolidate all common detach code in qemuDomainDetachDeviceLive qemu_hotplug: delay sending DEVICE_REMOVED event until after *all* teardown qemu_hotplug: don't shutdown net device until the guest has released it src/qemu/qemu_driver.c | 109 +---- src/qemu/qemu_hotplug.c | 919 +++++++++++++++++++--------------------- src/qemu/qemu_hotplug.h | 56 +-- tests/qemuhotplugtest.c | 8 +- 4 files changed, 465 insertions(+), 627 deletions(-) -- 2.20.1
This function is going to take on some of the functionality of its subordinate functions, which all live in qemu_hotplug.c. qemuDomainDetachDeviceControllerLive() is only called from qemuDomainDetachDeviceLive() (and will soon be merged into qemuDomainDetachControllerDevice(), which is in qemu_hotplug.c), so it is also moved. Signed-off-by: Laine Stump <laine@laine.org> --- Change from V1: take out changes of functions from global to static, move them to a separage patch (now in Patch 05/14) src/qemu/qemu_driver.c | 95 ---------------------------------------- src/qemu/qemu_hotplug.c | 97 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 5 +++ 3 files changed, 102 insertions(+), 95 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b27c6ce98e..6f5a4224c5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8001,101 +8001,6 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, return ret; } -static int -qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) -{ - virDomainControllerDefPtr cont = dev->data.controller; - int ret = -1; - - switch (cont->type) { - case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: - ret = qemuDomainDetachControllerDevice(driver, vm, dev, async); - break; - default : - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("'%s' controller cannot be hot unplugged."), - virDomainControllerTypeToString(cont->type)); - } - return ret; -} - -static int -qemuDomainDetachDeviceLive(virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - virQEMUDriverPtr driver, - bool async) -{ - int ret = -1; - - switch ((virDomainDeviceType)dev->type) { - case VIR_DOMAIN_DEVICE_DISK: - ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, async); - break; - case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainDetachDeviceControllerLive(driver, vm, dev, async); - break; - case VIR_DOMAIN_DEVICE_LEASE: - ret = qemuDomainDetachLease(driver, vm, dev->data.lease); - break; - case VIR_DOMAIN_DEVICE_NET: - ret = qemuDomainDetachNetDevice(driver, vm, dev, async); - break; - case VIR_DOMAIN_DEVICE_HOSTDEV: - ret = qemuDomainDetachHostDevice(driver, vm, dev, async); - break; - case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr, async); - break; - case VIR_DOMAIN_DEVICE_RNG: - ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng, async); - break; - case VIR_DOMAIN_DEVICE_MEMORY: - ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory, 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); - break; - case VIR_DOMAIN_DEVICE_INPUT: - ret = qemuDomainDetachInputDevice(vm, dev->data.input, async); - break; - case VIR_DOMAIN_DEVICE_REDIRDEV: - ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev, async); - break; - - case VIR_DOMAIN_DEVICE_VSOCK: - ret = qemuDomainDetachVsockDevice(vm, dev->data.vsock, async); - break; - - case VIR_DOMAIN_DEVICE_FS: - case VIR_DOMAIN_DEVICE_SOUND: - case VIR_DOMAIN_DEVICE_VIDEO: - case VIR_DOMAIN_DEVICE_GRAPHICS: - case VIR_DOMAIN_DEVICE_HUB: - case VIR_DOMAIN_DEVICE_SMARTCARD: - case VIR_DOMAIN_DEVICE_MEMBALLOON: - case VIR_DOMAIN_DEVICE_NVRAM: - case VIR_DOMAIN_DEVICE_NONE: - case VIR_DOMAIN_DEVICE_TPM: - case VIR_DOMAIN_DEVICE_PANIC: - case VIR_DOMAIN_DEVICE_IOMMU: - case VIR_DOMAIN_DEVICE_LAST: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("live detach of device '%s' is not supported"), - virDomainDeviceTypeToString(dev->type)); - break; - } - - if (ret == 0) - ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE); - - return ret; -} static int qemuDomainChangeDiskLive(virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c1d2d00016..7be0f341b7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6190,6 +6190,103 @@ qemuDomainDetachLease(virQEMUDriverPtr driver, } +static int +qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + bool async) +{ + virDomainControllerDefPtr cont = dev->data.controller; + int ret = -1; + + switch (cont->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: + ret = qemuDomainDetachControllerDevice(driver, vm, dev, async); + break; + default : + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("'%s' controller cannot be hot unplugged."), + virDomainControllerTypeToString(cont->type)); + } + return ret; +} + +int +qemuDomainDetachDeviceLive(virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + virQEMUDriverPtr driver, + bool async) +{ + int ret = -1; + + switch ((virDomainDeviceType)dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, async); + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + ret = qemuDomainDetachDeviceControllerLive(driver, vm, dev, async); + break; + case VIR_DOMAIN_DEVICE_LEASE: + ret = qemuDomainDetachLease(driver, vm, dev->data.lease); + break; + case VIR_DOMAIN_DEVICE_NET: + ret = qemuDomainDetachNetDevice(driver, vm, dev, async); + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + ret = qemuDomainDetachHostDevice(driver, vm, dev, async); + break; + case VIR_DOMAIN_DEVICE_CHR: + ret = qemuDomainDetachChrDevice(driver, vm, dev->data.chr, async); + break; + case VIR_DOMAIN_DEVICE_RNG: + ret = qemuDomainDetachRNGDevice(driver, vm, dev->data.rng, async); + break; + case VIR_DOMAIN_DEVICE_MEMORY: + ret = qemuDomainDetachMemoryDevice(driver, vm, dev->data.memory, 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); + break; + case VIR_DOMAIN_DEVICE_INPUT: + ret = qemuDomainDetachInputDevice(vm, dev->data.input, async); + break; + case VIR_DOMAIN_DEVICE_REDIRDEV: + ret = qemuDomainDetachRedirdevDevice(driver, vm, dev->data.redirdev, async); + break; + + case VIR_DOMAIN_DEVICE_VSOCK: + ret = qemuDomainDetachVsockDevice(vm, dev->data.vsock, async); + break; + + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_LAST: + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("live detach of device '%s' is not supported"), + virDomainDeviceTypeToString(dev->type)); + break; + } + + if (ret == 0) + ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE); + + return ret; +} + + static int qemuDomainRemoveVcpu(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 7ac03b7810..10593cf83c 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -158,6 +158,11 @@ int qemuDomainDetachRNGDevice(virQEMUDriverPtr driver, virDomainRNGDefPtr rng, bool async); +int qemuDomainDetachDeviceLive(virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + virQEMUDriverPtr driver, + bool async); + void qemuDomainRemoveVcpuAlias(virQEMUDriverPtr driver, virDomainObjPtr vm, const char *alias); -- 2.20.1
On Mon, Mar 25, 2019 at 13:24:23 -0400, Laine Stump wrote:
This function is going to take on some of the functionality of its subordinate functions, which all live in qemu_hotplug.c.
qemuDomainDetachDeviceControllerLive() is only called from qemuDomainDetachDeviceLive() (and will soon be merged into qemuDomainDetachControllerDevice(), which is in qemu_hotplug.c), so it is also moved.
Signed-off-by: Laine Stump <laine@laine.org> ---
ACK
qemuDomainDetachDeviceControllerLive() just checks if the controller type is SCSI, and then either returns failure, or calls qemuDomainDetachControllerDevice(). Instead, lets just check for type != SCSI at the top of the latter function, and call it directly. Signed-off-by: Laine Stump <laine@laine.org> --- Change from V1: remove unrelated code movement, per Peter's request. src/qemu/qemu_hotplug.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7be0f341b7..666657758e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5548,6 +5548,13 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, int idx, ret = -1; virDomainControllerDefPtr detach = NULL; + if (dev->data.controller->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("'%s' controller cannot be hot unplugged."), + virDomainControllerTypeToString(dev->data.controller->type)); + return -1; + } + if ((idx = virDomainControllerFind(vm->def, dev->data.controller->type, dev->data.controller->idx)) < 0) { @@ -6190,27 +6197,6 @@ qemuDomainDetachLease(virQEMUDriverPtr driver, } -static int -qemuDomainDetachDeviceControllerLive(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - bool async) -{ - virDomainControllerDefPtr cont = dev->data.controller; - int ret = -1; - - switch (cont->type) { - case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: - ret = qemuDomainDetachControllerDevice(driver, vm, dev, async); - break; - default : - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("'%s' controller cannot be hot unplugged."), - virDomainControllerTypeToString(cont->type)); - } - return ret; -} - int qemuDomainDetachDeviceLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev, @@ -6224,7 +6210,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, ret = qemuDomainDetachDeviceDiskLive(driver, vm, dev, async); break; case VIR_DOMAIN_DEVICE_CONTROLLER: - ret = qemuDomainDetachDeviceControllerLive(driver, vm, dev, async); + ret = qemuDomainDetachControllerDevice(driver, vm, dev, async); break; case VIR_DOMAIN_DEVICE_LEASE: ret = qemuDomainDetachLease(driver, vm, dev->data.lease); -- 2.20.1
On Mon, Mar 25, 2019 at 13:24:24 -0400, Laine Stump wrote:
qemuDomainDetachDeviceControllerLive() just checks if the controller type is SCSI, and then either returns failure, or calls qemuDomainDetachControllerDevice().
Instead, lets just check for type != SCSI at the top of the latter function, and call it directly.
Signed-off-by: Laine Stump <laine@laine.org> ---
ACK
qemuDomainDetachDeviceLive() is called from two places in qemu_driver.c, and qemuDomainUpdateDeviceList() is called from the end of qemuDomainDetachDeviceLive(), which is now in qemu_hotplug.c This patch replaces the single call to qemuDomainUpdateDeviceList() with two calls to it immediately after return from qemuDomainDetachDeviceLive(). This is only done if the return from that function is exactly 0, in order to exactly preserve previous behavior. Removing that one call from qemuDomainDetachDeviceList() will permit us to call it from the test driver hotplug test, replacing the separate calls to qemuDomainDetachDeviceDiskLive(), qemuDomainDetachChrDevice(), qemuDomainDetachShmemDevice() and qemuDomainDetachWatchdog(). We want to do this so that part of the common functionality of those three functions (and the rest of the device-specific Detach functions) can be pulled up into qemuDomainDetachDeviceLive() without breaking the test. (This is done in the next patch). NB: Almost certainly this is "not the best place" to call qemuDomainUpdateDeviceList() (actually, it is provably the *wrong* place), since it's purpose is to retrieve an "up to date" list of aliases for all devices from qemu, and if the guest OS hasn't yet processed the detach request, the now-being-removed device may still be on that list. It would arguably be better to instead call qemuDomainUpdateDevicesList() later during the response to the DEVICE_DELETED event for the device. But removing the call from the current point in the detach could have some unforeseen ill effect due to changed timing, so the change to move it into qemuDomainRemove*Device() will be done in a separate patch (in order to make it easily revertible in case it causes a regression). Signed-off-by: Laine Stump <laine@laine.org> --- Change from V1: * Added the big explanation above about why I'm not completely changing behavior by waiting unt after DEVICE_DELETED to call qemuDomainUpdateDeviceList() * Only call qemuDomainUpdateDeviceList() if the Detach function returned *exactly* 0, since there are cases where it could return > 0, and the previous behavior was to not call it in those cases. src/qemu/qemu_driver.c | 14 ++++++++++++-- src/qemu/qemu_hotplug.c | 3 --- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6f5a4224c5..51b434f0d2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8927,8 +8927,14 @@ qemuDomainDetachDeviceLiveAndConfig(virQEMUDriverPtr driver, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (qemuDomainDetachDeviceLive(vm, dev_copy, driver, false) < 0) + int rc; + + if ((rc = qemuDomainDetachDeviceLive(vm, dev_copy, driver, false)) < 0) + goto cleanup; + + if (rc == 0 && qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; + /* * update domain status forcibly because the domain status may be * changed even if we failed to attach the device. For example, @@ -9005,11 +9011,15 @@ qemuDomainDetachDeviceAliasLiveAndConfig(virQEMUDriverPtr driver, if (def) { virDomainDeviceDef dev; + int rc; if (virDomainDefFindDevice(def, alias, &dev, true) < 0) goto cleanup; - if (qemuDomainDetachDeviceLive(vm, &dev, driver, true) < 0) + if ((rc = qemuDomainDetachDeviceLive(vm, &dev, driver, true)) < 0) + goto cleanup; + + if (rc == 0 && qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) goto cleanup; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 666657758e..b435ea99ac 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6266,9 +6266,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, break; } - if (ret == 0) - ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE); - return ret; } -- 2.20.1
On Mon, Mar 25, 2019 at 13:24:25 -0400, Laine Stump wrote:
qemuDomainDetachDeviceLive() is called from two places in qemu_driver.c, and qemuDomainUpdateDeviceList() is called from the end of qemuDomainDetachDeviceLive(), which is now in qemu_hotplug.c
This patch replaces the single call to qemuDomainUpdateDeviceList() with two calls to it immediately after return from qemuDomainDetachDeviceLive(). This is only done if the return from that function is exactly 0, in order to exactly preserve previous behavior.
Removing that one call from qemuDomainDetachDeviceList() will permit us to call it from the test driver hotplug test, replacing the separate calls to qemuDomainDetachDeviceDiskLive(), qemuDomainDetachChrDevice(), qemuDomainDetachShmemDevice() and qemuDomainDetachWatchdog(). We want to do this so that part of the common functionality of those three functions (and the rest of the device-specific Detach functions) can be pulled up into qemuDomainDetachDeviceLive() without breaking the test. (This is done in the next patch).
NB: Almost certainly this is "not the best place" to call qemuDomainUpdateDeviceList() (actually, it is provably the *wrong* place), since it's purpose is to retrieve an "up to date" list of aliases for all devices from qemu, and if the guest OS hasn't yet processed the detach request, the now-being-removed device may still be on that list. It would arguably be better to instead call qemuDomainUpdateDevicesList() later during the response to the DEVICE_DELETED event for the device. But removing the call from the current point in the detach could have some unforeseen ill effect due to changed timing, so the change to move it into qemuDomainRemove*Device() will be done in a separate patch (in order to make it easily revertible in case it causes a regression).
Actually it probably would cause a regression. It's called only when the device is removed. That function will require some overhauling.
Signed-off-by: Laine Stump <laine@laine.org> ---
Change from V1:
* Added the big explanation above about why I'm not completely changing behavior by waiting unt after DEVICE_DELETED to call qemuDomainUpdateDeviceList()
* Only call qemuDomainUpdateDeviceList() if the Detach function returned *exactly* 0, since there are cases where it could return > 0, and the previous behavior was to not call it in those cases.
ACK
On 3/26/19 3:45 AM, Peter Krempa wrote:
On Mon, Mar 25, 2019 at 13:24:25 -0400, Laine Stump wrote:
qemuDomainDetachDeviceLive() is called from two places in qemu_driver.c, and qemuDomainUpdateDeviceList() is called from the end of qemuDomainDetachDeviceLive(), which is now in qemu_hotplug.c
This patch replaces the single call to qemuDomainUpdateDeviceList() with two calls to it immediately after return from qemuDomainDetachDeviceLive(). This is only done if the return from that function is exactly 0, in order to exactly preserve previous behavior.
Removing that one call from qemuDomainDetachDeviceList() will permit us to call it from the test driver hotplug test, replacing the separate calls to qemuDomainDetachDeviceDiskLive(), qemuDomainDetachChrDevice(), qemuDomainDetachShmemDevice() and qemuDomainDetachWatchdog(). We want to do this so that part of the common functionality of those three functions (and the rest of the device-specific Detach functions) can be pulled up into qemuDomainDetachDeviceLive() without breaking the test. (This is done in the next patch).
NB: Almost certainly this is "not the best place" to call qemuDomainUpdateDeviceList() (actually, it is provably the *wrong* place), since it's purpose is to retrieve an "up to date" list of aliases for all devices from qemu, and if the guest OS hasn't yet processed the detach request, the now-being-removed device may still be on that list. It would arguably be better to instead call qemuDomainUpdateDevicesList() later during the response to the DEVICE_DELETED event for the device. But removing the call from the current point in the detach could have some unforeseen ill effect due to changed timing, so the change to move it into qemuDomainRemove*Device() will be done in a separate patch (in order to make it easily revertible in case it causes a regression). Actually it probably would cause a regression. It's called only when the device is removed. That function will require some overhauling.
Yeah, I started to look at it and decided it was too complicated to try and pull together in a few minutes while I was rushed, so I just wrote it on my to-do list.
Signed-off-by: Laine Stump <laine@laine.org> ---
Change from V1:
* Added the big explanation above about why I'm not completely changing behavior by waiting unt after DEVICE_DELETED to call qemuDomainUpdateDeviceList()
* Only call qemuDomainUpdateDeviceList() if the Detach function returned *exactly* 0, since there are cases where it could return > 0, and the previous behavior was to not call it in those cases. ACK
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@laine.org> --- 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
On Mon, Mar 25, 2019 at 13:24:26 -0400, Laine Stump wrote:
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@laine.org> ---
ACK
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@laine.org> --- 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
On Mon, Mar 25, 2019 at 13:24:27 -0400, Laine Stump wrote:
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@laine.org> ---
NEW PATCH IN V2
src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++------------------ src/qemu/qemu_hotplug.h | 51 ----------------------------------------- 2 files changed, 25 insertions(+), 73 deletions(-)
ACK
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@laine.org> --- 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
On Mon, Mar 25, 2019 at 13:24:28 -0400, Laine Stump wrote:
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@laine.org> ---
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(-)
ACK
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@laine.org> --- 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
On Mon, Mar 25, 2019 at 13:24:29 -0400, Laine Stump wrote:
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@laine.org> ---
ACK
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@laine.org> --- 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 Mon, Mar 25, 2019 at 13:24:30 -0400, Laine Stump wrote:
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
I must say that doing this along with the rename did not help reviewability. The rename which includes whitespace change in the argument list obscures actual argument changes. I'd prefer if you did not do that in the future as I understand that splitting this patch would be an even bigger nightmare.
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@laine.org> ---
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(-)
[...]
@@ -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; + +
extra line
+ *detach = watchdog = vm->def->watchdog;
if (!watchdog) { virReportError(VIR_ERR_DEVICE_MISSING, "%s",
[...]
@@ -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; + }
I'm not a fan of the curly braces on this single-line body but I was told that the coding style mandates it. Thus I'll not require a change here. Also, all the functions should use the qemuHotplug prefix rather than qemuDomain, but given that the file is inconsistent I'm not going to insist. ACK witht the extra line deleted. Please don't send further patches which mix function renames and argument list change.
On 3/26/19 8:40 AM, Peter Krempa wrote:
On Mon, Mar 25, 2019 at 13:24:30 -0400, Laine Stump wrote:
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 I must say that doing this along with the rename did not help reviewability. The rename which includes whitespace change in the argument list obscures actual argument changes.
I'd prefer if you did not do that in the future as I understand that splitting this patch would be an even bigger nightmare.
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!
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@laine.org> ---
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(-) [...]
@@ -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; + + extra line
+ *detach = watchdog = vm->def->watchdog;
if (!watchdog) { virReportError(VIR_ERR_DEVICE_MISSING, "%s", [...]
@@ -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; + } I'm not a fan of the curly braces on this single-line body but I was told that the coding style mandates it. Thus I'll not require a change here.
I'm not a fan of *not* putting curly braces here :-)
Also, all the functions should use the qemuHotplug prefix rather than qemuDomain, but given that the file is inconsistent I'm not going to insist.
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...
ACK witht the extra line deleted. Please don't send further patches which mix function renames and argument list change.
Yes sir!
qemuDomainDetachDeviceChr and qemuDomainDetachDeviceLease are more consistent with each other. Signed-off-by: Laine Stump <laine@laine.org> --- NEW PATCH in V2 (previously was part of 08/14) src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0c8e673381..de30b08dd1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5945,7 +5945,7 @@ qemuDomainDetachPrepNet(virQEMUDriverPtr driver, static int -qemuDomainDetachChrDevice(virQEMUDriverPtr driver, +qemuDomainDetachDeviceChr(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainChrDefPtr chr, bool async) @@ -6189,9 +6189,9 @@ qemuDomainDetachPrepVsock(virDomainObjPtr vm, static int -qemuDomainDetachLease(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainLeaseDefPtr lease) +qemuDomainDetachDeviceLease(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainLeaseDefPtr lease) { virDomainLeaseDefPtr det_lease; int idx; @@ -6228,10 +6228,10 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, * Detach functions. */ case VIR_DOMAIN_DEVICE_LEASE: - return qemuDomainDetachLease(driver, vm, match->data.lease); + return qemuDomainDetachDeviceLease(driver, vm, match->data.lease); case VIR_DOMAIN_DEVICE_CHR: - return qemuDomainDetachChrDevice(driver, vm, match->data.chr, async); + return qemuDomainDetachDeviceChr(driver, vm, match->data.chr, async); /* * All the other device types follow a very similar pattern - -- 2.20.1
On Mon, Mar 25, 2019 at 13:24:31 -0400, Laine Stump wrote:
qemuDomainDetachDeviceChr and qemuDomainDetachDeviceLease are more consistent with each other.
Signed-off-by: Laine Stump <laine@laine.org> ---
NEW PATCH in V2 (previously was part of 08/14)
src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
ACK
This function can be called with a virDomainDevicePtr and whether or not the removal was successful, and it will call the appropriate virDomainAudit*() function with the appropriate args for whatever type of device it's given (or do nothing, if that's appropriate). This permits generalizing some code that currently has a separate copy for each type of device. NB: Although the function initially will be called only with success=false, that has been made an argument so that in the future (when the qemuDomainRemove*Device() functions have had their common functionality consolidated into qemuDomainRemoveDevice()), this new common code can call qemuDomainRemoveAuditDevice() for all types. Signed-off-by: Laine Stump <laine@laine.org> --- change from V1: * only audit device types that were previously audited on *failure* to remove (this preserves previous behavior). Auditing of other device types is now added in new patch 11/14. * use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error due to the new function not yet being used. src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index de30b08dd1..92d4e7d0f9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5208,6 +5208,61 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, } +static void ATTRIBUTE_UNUSED +qemuDomainRemoveAuditDevice(virDomainObjPtr vm, + virDomainDeviceDefPtr detach, + bool success) +{ + switch ((virDomainDeviceType)detach->type) { + case VIR_DOMAIN_DEVICE_DISK: + virDomainAuditDisk(vm, detach->data.disk->src, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_NET: + virDomainAuditNet(vm, detach->data.net, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success); + break; + + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_REDIRDEV: + /* + * These devices are supposed to be audited, but current code + * doesn't audit on failure to remove the device. + */ + break; + + + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_VSOCK: + /* These devices don't have associated audit logs */ + break; + + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_LAST: + /* these will never happen, these devices can't be unplugged */ + break; + } +} + + int qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, -- 2.20.1
On Mon, Mar 25, 2019 at 13:24:32 -0400, Laine Stump wrote:
This function can be called with a virDomainDevicePtr and whether or not the removal was successful, and it will call the appropriate virDomainAudit*() function with the appropriate args for whatever type of device it's given (or do nothing, if that's appropriate). This permits generalizing some code that currently has a separate copy for each type of device.
NB: Although the function initially will be called only with success=false, that has been made an argument so that in the future (when the qemuDomainRemove*Device() functions have had their common functionality consolidated into qemuDomainRemoveDevice()), this new common code can call qemuDomainRemoveAuditDevice() for all types.
Signed-off-by: Laine Stump <laine@laine.org> ---
change from V1:
* only audit device types that were previously audited on *failure* to remove (this preserves previous behavior). Auditing of other device types is now added in new patch 11/14.
* use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error due to the new function not yet being used.
src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index de30b08dd1..92d4e7d0f9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5208,6 +5208,61 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, }
+static void ATTRIBUTE_UNUSED +qemuDomainRemoveAuditDevice(virDomainObjPtr vm, + virDomainDeviceDefPtr detach, + bool success) +{ + switch ((virDomainDeviceType)detach->type) { + case VIR_DOMAIN_DEVICE_DISK: + virDomainAuditDisk(vm, detach->data.disk->src, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_NET: + virDomainAuditNet(vm, detach->data.net, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success); + break; + + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_REDIRDEV: + /* + * These devices are supposed to be audited, but current code + * doesn't audit on failure to remove the device. + */ + break; + + + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_CONTROLLER: + case VIR_DOMAIN_DEVICE_WATCHDOG: + case VIR_DOMAIN_DEVICE_VSOCK: + /* These devices don't have associated audit logs */ + break; + + case VIR_DOMAIN_DEVICE_FS: + case VIR_DOMAIN_DEVICE_SOUND: + case VIR_DOMAIN_DEVICE_VIDEO: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_HUB: + case VIR_DOMAIN_DEVICE_SMARTCARD: + case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: + case VIR_DOMAIN_DEVICE_NONE: + case VIR_DOMAIN_DEVICE_TPM: + case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_IOMMU: + case VIR_DOMAIN_DEVICE_LAST: + /* these will never happen, these devices can't be unplugged */
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 Mon, Mar 25, 2019 at 01:24:32PM -0400, Laine Stump wrote:
This function can be called with a virDomainDevicePtr and whether or not the removal was successful, and it will call the appropriate virDomainAudit*() function with the appropriate args for whatever type of device it's given (or do nothing, if that's appropriate). This permits generalizing some code that currently has a separate copy for each type of device.
NB: Although the function initially will be called only with success=false, that has been made an argument so that in the future (when the qemuDomainRemove*Device() functions have had their common functionality consolidated into qemuDomainRemoveDevice()), this new common code can call qemuDomainRemoveAuditDevice() for all types.
Signed-off-by: Laine Stump <laine@laine.org> ---
change from V1:
* only audit device types that were previously audited on *failure* to remove (this preserves previous behavior). Auditing of other device types is now added in new patch 11/14.
* use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error due to the new function not yet being used.
src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index de30b08dd1..92d4e7d0f9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5208,6 +5208,61 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, }
+static void ATTRIBUTE_UNUSED +qemuDomainRemoveAuditDevice(virDomainObjPtr vm, + virDomainDeviceDefPtr detach, + bool success) +{ + switch ((virDomainDeviceType)detach->type) { + case VIR_DOMAIN_DEVICE_DISK: + virDomainAuditDisk(vm, detach->data.disk->src, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_NET: + virDomainAuditNet(vm, detach->data.net, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success); + break; + + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_REDIRDEV: + /* + * These devices are supposed to be audited, but current code + * doesn't audit on failure to remove the device. + */ + break;
Indentation is off here. Jano
On 3/26/19 12:58 PM, Ján Tomko wrote:
On Mon, Mar 25, 2019 at 01:24:32PM -0400, Laine Stump wrote:
This function can be called with a virDomainDevicePtr and whether or not the removal was successful, and it will call the appropriate virDomainAudit*() function with the appropriate args for whatever type of device it's given (or do nothing, if that's appropriate). This permits generalizing some code that currently has a separate copy for each type of device.
NB: Although the function initially will be called only with success=false, that has been made an argument so that in the future (when the qemuDomainRemove*Device() functions have had their common functionality consolidated into qemuDomainRemoveDevice()), this new common code can call qemuDomainRemoveAuditDevice() for all types.
Signed-off-by: Laine Stump <laine@laine.org> ---
change from V1:
* only audit device types that were previously audited on *failure* to remove (this preserves previous behavior). Auditing of other device types is now added in new patch 11/14.
* use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error due to the new function not yet being used.
src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index de30b08dd1..92d4e7d0f9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5208,6 +5208,61 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver, }
+static void ATTRIBUTE_UNUSED +qemuDomainRemoveAuditDevice(virDomainObjPtr vm, + virDomainDeviceDefPtr detach, + bool success) +{ + switch ((virDomainDeviceType)detach->type) { + case VIR_DOMAIN_DEVICE_DISK: + virDomainAuditDisk(vm, detach->data.disk->src, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_NET: + virDomainAuditNet(vm, detach->data.net, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success); + break; + + case VIR_DOMAIN_DEVICE_INPUT: + case VIR_DOMAIN_DEVICE_CHR: + case VIR_DOMAIN_DEVICE_RNG: + case VIR_DOMAIN_DEVICE_MEMORY: + case VIR_DOMAIN_DEVICE_SHMEM: + case VIR_DOMAIN_DEVICE_REDIRDEV: + /* + * These devices are supposed to be audited, but current code + * doesn't audit on failure to remove the device. + */ + break;
Indentation is off here.
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!
Although all hotpluggable devices other than lease, controller, watchdof, and vsock can be audited, and *are* audited when an unplug is successful, only disk, net, and hostdev were actually being audited on failure. This patch corrects that omission. Signed-off-by: Laine Stump <laine@laine.org> --- NEW PATCH in V2 - previously a part of patch 10/14 src/qemu/qemu_hotplug.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 92d4e7d0f9..e9d6c8622b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5223,19 +5223,28 @@ qemuDomainRemoveAuditDevice(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_HOSTDEV: virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success); break; - case VIR_DOMAIN_DEVICE_INPUT: + virDomainAuditInput(vm, detach->data.input, "detach", success); + break; case VIR_DOMAIN_DEVICE_CHR: + virDomainAuditChardev(vm, detach->data.chr, NULL, "detach", success); + break; case VIR_DOMAIN_DEVICE_RNG: - case VIR_DOMAIN_DEVICE_MEMORY: + virDomainAuditRNG(vm, detach->data.rng, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_MEMORY: { + unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def); + unsigned long long newmem = oldmem - detach->data.memory->size; + + virDomainAuditMemory(vm, oldmem, newmem, "update", success); + break; + } case VIR_DOMAIN_DEVICE_SHMEM: + virDomainAuditShmem(vm, detach->data.shmem, "detach", success); + break; case VIR_DOMAIN_DEVICE_REDIRDEV: - /* - * These devices are supposed to be audited, but current code - * doesn't audit on failure to remove the device. - */ - break; - + virDomainAuditRedirdev(vm, detach->data.redirdev, "detach", success); + break; case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_CONTROLLER: -- 2.20.1
On Mon, Mar 25, 2019 at 13:24:33 -0400, Laine Stump wrote:
Although all hotpluggable devices other than lease, controller, watchdof, and vsock can be audited, and *are* audited when an unplug is successful, only disk, net, and hostdev were actually being audited on failure.
This patch corrects that omission.
Signed-off-by: Laine Stump <laine@laine.org> ---
NEW PATCH in V2 - previously a part of patch 10/14
src/qemu/qemu_hotplug.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 92d4e7d0f9..e9d6c8622b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5223,19 +5223,28 @@ qemuDomainRemoveAuditDevice(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_HOSTDEV: virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success); break; - case VIR_DOMAIN_DEVICE_INPUT: + virDomainAuditInput(vm, detach->data.input, "detach", success); + break; case VIR_DOMAIN_DEVICE_CHR: + virDomainAuditChardev(vm, detach->data.chr, NULL, "detach", success); + break; case VIR_DOMAIN_DEVICE_RNG: - case VIR_DOMAIN_DEVICE_MEMORY: + virDomainAuditRNG(vm, detach->data.rng, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_MEMORY: { + unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def); + unsigned long long newmem = oldmem - detach->data.memory->size; + + virDomainAuditMemory(vm, oldmem, newmem, "update", success);
This probably should also say "detach" as the rest does. ACK
On 3/26/19 8:52 AM, Peter Krempa wrote:
On Mon, Mar 25, 2019 at 13:24:33 -0400, Laine Stump wrote:
Although all hotpluggable devices other than lease, controller, watchdof, and vsock can be audited, and *are* audited when an unplug is successful, only disk, net, and hostdev were actually being audited on failure.
This patch corrects that omission.
Signed-off-by: Laine Stump <laine@laine.org> ---
NEW PATCH in V2 - previously a part of patch 10/14
src/qemu/qemu_hotplug.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 92d4e7d0f9..e9d6c8622b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5223,19 +5223,28 @@ qemuDomainRemoveAuditDevice(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_HOSTDEV: virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success); break; - case VIR_DOMAIN_DEVICE_INPUT: + virDomainAuditInput(vm, detach->data.input, "detach", success); + break; case VIR_DOMAIN_DEVICE_CHR: + virDomainAuditChardev(vm, detach->data.chr, NULL, "detach", success); + break; case VIR_DOMAIN_DEVICE_RNG: - case VIR_DOMAIN_DEVICE_MEMORY: + virDomainAuditRNG(vm, detach->data.rng, NULL, "detach", success); + break; + case VIR_DOMAIN_DEVICE_MEMORY: { + unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def); + unsigned long long newmem = oldmem - detach->data.memory->size; + + virDomainAuditMemory(vm, oldmem, newmem, "update", success); This probably should also say "detach" as the rest does.
...except that all the other memory audits always say "update" rather than "detach" or "attach". Maybe the author decided to look at "memory" as a single entity that could get larger or smaller, rather than a collection of several different distinct
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@laine.org> --- 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
On Mon, Mar 25, 2019 at 13:24:34 -0400, Laine Stump wrote:
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@laine.org> ---
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
[...]
- goto cleanup; - } - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveNetDevice(driver, vm, net); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; }
-
We usually do two lines between functions so there's no need to break it here.
static int qemuDomainDetachDeviceChr(virQEMUDriverPtr driver, virDomainObjPtr vm,
ACK
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@laine.org> --- 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
On Mon, Mar 25, 2019 at 13:24:35 -0400, Laine Stump wrote:
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@laine.org> ---
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(-)
[...]
@@ -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.
I think this should only say that this function needs to report the event itself. Also mention that it has to happen after all the backend stuff is detached. /* The caller does not emit the event. Note that the event should be * reported only after all backend stuff is gone */
+ */ event = virDomainEventDeviceRemovedNewFromObj(vm, chr->info.alias); virObjectEventStateQueue(driver->domainEventState, event);
- qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); - qemuDomainChrRemove(vm->def, chr); virDomainChrDefFree(chr); ret = 0;
[...]
@@ -5277,50 +5237,79 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDeviceDefPtr dev) { - int ret = -1; + virDomainDeviceInfoPtr info; + virObjectEventPtr event; + VIR_AUTOFREE(char *)alias = NULL;
Missing space before 'alias'
+ + /* + * 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) {
&& is usually at the end of the previous line.
+ return -1; + } + info = NULL; /* to prevent accidental use later */
// this is bridge [1]
+ switch ((virDomainDeviceType)dev->type) {
ACK with the above addressed [1] https://imgur.com/gallery/dZe8k
On Tue, Mar 26, 2019 at 01:15:33PM +0100, Peter Krempa wrote:
On Mon, Mar 25, 2019 at 13:24:35 -0400, Laine Stump wrote:
+ return -1; + } + info = NULL; /* to prevent accidental use later */
// this is bridge [1]
+ switch ((virDomainDeviceType)dev->type) {
ACK with the above addressed
Now with more credit to the author: https://www.abstrusegoose.com/432 Jano
For [some unknown reason, possibly/probably pure chance], Net devices have been taken offline and their bandwidth tc rules cleared as the very first operation when detaching the device. This is contrary to every other type of device, where all hostside teardown is delayed until we receive the DEVICE_DELETED event back from qemu, indicating that the guest has finished with the device. This patch delays these two operations until receipt of DEVICE_DELETED, which removes an ugly wart from qemuDomainDetachDeviceLive(), and also seems to be a more correct sequence of events. Signed-off-by: Laine Stump <laine@laine.org> --- NEW PATCH IN V2. src/qemu/qemu_hotplug.c | 49 +++++++++-------------------------------- 1 file changed, 11 insertions(+), 38 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 78890f4bbe..2e2e913b4e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4821,6 +4821,17 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, !(charDevAlias = qemuAliasChardevFromDevAlias(net->info.alias))) goto cleanup; + if (virDomainNetGetActualBandwidth(net) && + virNetDevSupportBandwidth(virDomainNetGetActualType(net)) && + virNetDevBandwidthClear(net->ifname) < 0) + VIR_WARN("cannot clear bandwidth setting for device : %s", + 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(net)); qemuDomainObjEnterMonitor(driver, vm); if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) { @@ -5795,33 +5806,6 @@ qemuDomainDetachPrepNet(virDomainObjPtr vm, } -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) - VIR_WARN("cannot clear bandwidth setting for device : %s", - 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(net)); -} - static int qemuDomainDetachDeviceChr(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -6152,17 +6136,6 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, 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 -- 2.20.1
On Mon, Mar 25, 2019 at 13:24:36 -0400, Laine Stump wrote:
For [some unknown reason, possibly/probably pure chance], Net devices have been taken offline and their bandwidth tc rules cleared as the very first operation when detaching the device. This is contrary to every other type of device, where all hostside teardown is delayed until we receive the DEVICE_DELETED event back from qemu, indicating that the guest has finished with the device.
This patch delays these two operations until receipt of DEVICE_DELETED, which removes an ugly wart from qemuDomainDetachDeviceLive(), and also seems to be a more correct sequence of events.
Signed-off-by: Laine Stump <laine@laine.org> ---
NEW PATCH IN V2.
Looks good to me although I don't have much knowledge in this regions. Michal, could you please have a look?
participants (3)
-
Ján Tomko -
Laine Stump -
Peter Krempa