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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux