Re: [libvirt] [PATCH 18/21] qemu_hotplug: standardize the names/args/calling of qemuDomainDetach*()

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(a)laine.org&gt; > --- > 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 qemuDomainDetachDiskDevice > int 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.

> > - 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.

> > - 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'.

> return ret; > } > > -- > 2.20.1 > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list