[libvirt] [PATCH v1 1/3] qemu_conf.c: introduce qemuAddRemoveSharedHostdevInternal

[Pavel Hrdina]

On Tue, Sep 03, 2019 at 08:06:05PM -0300, Daniel Henrique Barboza wrote:
> qemuAddSharedHostdev() has a code similar to
> qemuRemoveSharedHostdev(), with exception of one line that
> defines the operation (add or remove).
> 
> This patch introduces a new function that aggregates the common
> code, using a flag to switch between the operations, avoiding
> code repetition.
> 
> No functional change was made.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
> ---
>  src/qemu/qemu_conf.c | 93 ++++++++++++++++++--------------------------
>  1 file changed, 37 insertions(+), 56 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index d771bb6916..a583440807 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1722,9 +1722,33 @@ qemuGetHostdevPath(virDomainHostdevDefPtr hostdev)
>  
>  
>  static int
> -qemuAddSharedHostdev(virQEMUDriverPtr driver,
> -                     virDomainHostdevDefPtr hostdev,
> -                     const char *name)
> +qemuSharedDeviceEntryRemove(virQEMUDriverPtr driver,
> +                            const char *key,
> +                            const char *name)
> +{
> +    qemuSharedDeviceEntryPtr entry = NULL;
> +    int idx;
> +
> +    if (!(entry = virHashLookup(driver->sharedDevices, key)))
> +        return -1;
> +
> +    /* Nothing to do if the shared disk is not recored in the table. */
> +    if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx))
> +        return 0;
> +
> +    if (entry->ref != 1)
> +        VIR_DELETE_ELEMENT(entry->domains, idx, entry->ref);
> +    else
> +        ignore_value(virHashRemoveEntry(driver->sharedDevices, key));
> +
> +    return 0;
> +}
> +
> +
> +static int
> +qemuAddRemoveSharedHostdevInternal(virQEMUDriverPtr driver,
> +                                   virDomainHostdevDefPtr hostdev,
> +                                   const char *name, bool addDevice)

We prefer to have each parameter on separate line, the only case were
having them on a single line is for files where that code style is the
main one.

There is no need to name the function as Internal, it would make sense
only in case where you would keep the original functions
qemuAddSharedHostdev and qemuSharedDeviceEntryRemove where they would
internally call the new function with proper flag set.

Pavel

>  {
>      char *dev_path = NULL;
>      char *key = NULL;
> @@ -1740,37 +1764,19 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver,
>          goto cleanup;
>  
>      qemuDriverLock(driver);
> -    ret = qemuSharedDeviceEntryInsert(driver, key, name);
> +
> +    if (addDevice)
> +        ret = qemuSharedDeviceEntryInsert(driver, key, name);
> +    else
> +        ret = qemuSharedDeviceEntryRemove(driver, key, name);
> +
>      qemuDriverUnlock(driver);
>  
>   cleanup:
>      VIR_FREE(dev_path);
>      VIR_FREE(key);
>      return ret;
> -}
> -
> -
> -static int
> -qemuSharedDeviceEntryRemove(virQEMUDriverPtr driver,
> -                            const char *key,
> -                            const char *name)
> -{
> -    qemuSharedDeviceEntryPtr entry = NULL;
> -    int idx;
> -
> -    if (!(entry = virHashLookup(driver->sharedDevices, key)))
> -        return -1;
> -
> -    /* Nothing to do if the shared disk is not recored in the table. */
> -    if (!qemuSharedDeviceEntryDomainExists(entry, name, &idx))
> -        return 0;
>  
> -    if (entry->ref != 1)
> -        VIR_DELETE_ELEMENT(entry->domains, idx, entry->ref);
> -    else
> -        ignore_value(virHashRemoveEntry(driver->sharedDevices, key));
> -
> -    return 0;
>  }
>  
>  
> @@ -1795,7 +1801,8 @@ qemuAddSharedDevice(virQEMUDriverPtr driver,
>      if (dev->type == VIR_DOMAIN_DEVICE_DISK)
>          return qemuAddSharedDisk(driver, dev->data.disk, name);
>      else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
> -        return qemuAddSharedHostdev(driver, dev->data.hostdev, name);
> +        return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
> +                                                  name, true);
>      else
>          return 0;
>  }
> @@ -1830,33 +1837,6 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver,
>  }
>  
>  
> -static int
> -qemuRemoveSharedHostdev(virQEMUDriverPtr driver,
> -                        virDomainHostdevDefPtr hostdev,
> -                        const char *name)
> -{
> -    char *dev_path = NULL;
> -    char *key = NULL;
> -    int ret = -1;
> -
> -    if (!qemuIsSharedHostdev(hostdev))
> -        return 0;
> -
> -    if (!(dev_path = qemuGetHostdevPath(hostdev)))
> -        goto cleanup;
> -
> -    if (!(key = qemuGetSharedDeviceKey(dev_path)))
> -        goto cleanup;
> -
> -    qemuDriverLock(driver);
> -    ret = qemuSharedDeviceEntryRemove(driver, key, name);
> -    qemuDriverUnlock(driver);
> -
> - cleanup:
> -    VIR_FREE(dev_path);
> -    VIR_FREE(key);
> -    return ret;
> -}
>  
>  
>  /* qemuRemoveSharedDevice:
> @@ -1876,7 +1856,8 @@ qemuRemoveSharedDevice(virQEMUDriverPtr driver,
>      if (dev->type == VIR_DOMAIN_DEVICE_DISK)
>          return qemuRemoveSharedDisk(driver, dev->data.disk, name);
>      else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV)
> -        return qemuRemoveSharedHostdev(driver, dev->data.hostdev, name);
> +        return qemuAddRemoveSharedHostdevInternal(driver, dev->data.hostdev,
> +                                                  name, false);
>      else
>          return 0;
>  }
> -- 
> 2.21.0
> 
> --
> 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/20190909/4f3b0d87/attachment-0001.sig>