On 3/22/19 8:10 AM, Peter Krempa wrote:
On Thu, Mar 21, 2019 at 18:28:58 -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. Two of the functions (for Lease and Chr) are atypical, and can't be easily consolidated with the others, so they are just being named qemuDomainDetachDevice*() to make it clearer that they perform the entire operation and not just some setup.You can do this separately.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@xxxxxxxxx> --- src/qemu/qemu_hotplug.c | 363 +++++++++++++++++++++++----------------- 1 file changed, 205 insertions(+), 158 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 903a0c46eb..b0e2c738b9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5390,27 +5390,28 @@ 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 detach; + virDomainDiskDefPtr disk;This reverts change done in previous commit of this series: qemu_hotplug: refactor qemuDomainDetachDiskLive and qemuDomainDetachDiskDeviceint idx; int ret = -1;- if ((idx = qemuFindDisk(vm->def, dev->data.disk->dst)) < 0) {+ if ((idx = qemuFindDisk(vm->def, match->dst)) < 0) {[...]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; + virDomainShmemDefPtr shmem;I don't see a point in removing initialization of the variable.
Well... I don't see the point in *keeping* it. :-P It's never used without being set, so it's pointless. And not having a NULL initialization makes it more consistent with the functions for other device types.
- 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:[...]@@ -5875,53 +5881,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;Same here, I don't see the reason to stop initializing it to NULL.
Same comment as above. But if you're really attached to it, then I'll continue initializing it to NULL.
- if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0)+ if ((detachidx = virDomainNetFindIdx(vm->def, match)) < 0) goto cleanup;[...]@@ -6180,9 +6192,9 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm,static int-qemuDomainDetachLease(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainLeaseDefPtr lease) +qemuDomainDetachDeviceLease(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainLeaseDefPtr lease) { virDomainLeaseDefPtr det_lease; int idx; @@ -6209,6 +6221,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, virQEMUDriverPtr driver, bool async) { + virDomainDeviceDef detach = { .type = match->type }; int ret = -1;switch ((virDomainDeviceType)match->type) {@@ -6218,10 +6231,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 - @@ -6231,38 +6244,70 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, * 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:@@ -6284,6 +6329,8 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, return -1; }+ ret = 0;+It does not seem to make sense to have 'ret' here after you removed use of 'ret'.
When more code is added here in patch 20, ret will once again be used. I figured it would be better to leave it in, rather than remove it just to add it back. I can do that if you want though.
return ret; }--2.20.1 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
