[libvirt] [PATCH 18/21] qemu_hotplug: standardize the names/args/calling of qemuDomainDetach*()
Laine Stump
laine at laine.org
Fri Mar 22 13:52:10 UTC 2019
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 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.
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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list