[libvirt] [PATCH 18/21] qemu_hotplug: standardize the names/args/calling of qemuDomainDetach*()
Peter Krempa
pkrempa at redhat.com
Fri Mar 22 12:10:28 UTC 2019
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 at laine.org>
> ---
> 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190322/cc729fbc/attachment-0001.sig>
More information about the libvir-list
mailing list