[libvirt] [PATCH 2/6] qemu: Merge qemuCheckSharedDisk into qemuAddSharedDisk

[John Ferlan]

On 02/13/2013 09:57 AM, Osier Yang wrote:
> Based on moving various checking into qemuAddSharedDisk, this
> avoids the caller using it in wrong ways.
> ---
>  src/qemu/qemu_conf.c    |   66 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_driver.c  |    5 ---
>  src/qemu/qemu_process.c |   53 -------------------------------------
>  src/qemu/qemu_process.h |    3 --
>  4 files changed, 66 insertions(+), 61 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index d0ee80b..6e735c6 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -821,6 +821,69 @@ qemuGetSharedDiskKey(const char *disk_path)
>      return key;
>  }
>  
> +/* Check if a shared disk's setting conflicts with the conf
> + * used by other domain(s). Currently only checks the sgio
> + * setting. Note that this should only be called for disk with
> + * block source.
> + *
> + * Returns 0 if no conflicts, otherwise returns -1.
> + */
> +static int
> +qemuCheckSharedDisk(virHashTablePtr sharedDisks,
> +                    virDomainDiskDefPtr disk)
> +{
> +    int val;
> +    size_t *ref = NULL;
> +    char *sysfs_path = NULL;
> +    char *key = NULL;
> +    int ret = 0;
> +
> +    if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL))) {
> +        ret = -1;
> +        goto cleanup;
> +    }
> +
> +    /* It can't be conflict if unpriv_sgio is not supported
> +     * by kernel.
> +     */
> +    if (!virFileExists(sysfs_path))
> +        goto cleanup;
> +
> +
> +    if (!(key = qemuGetSharedDiskKey(disk->src))) {
> +        ret = -1;
> +        goto cleanup;
> +    }
> +
> +    /* It can't be conflict if no other domain is
> +     * is sharing it.
> +     */
> +    if (!(ref = virHashLookup(sharedDisks, key)))
> +        goto cleanup;

The code you deleted below checked if (ref == (void)0x1) - so that's not
necessary now?  "ref" isn't used as far as I can see (oh and of course
the next patch removes that).

> +
> +    if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
> +        ret = -1;
> +        goto cleanup;
> +    }
> +
> +    if ((val == 0 &&
> +         (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
> +          disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
> +        (val == 1 &&
> +         disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
> +        goto cleanup;
> +
> +    virReportError(VIR_ERR_OPERATION_INVALID,
> +                   _("sgio of shared disk '%s' conflicts with other "
> +                     "active domains"), disk->src);
> +    ret = -1;
> +
> +cleanup:
> +    VIR_FREE(sysfs_path);
> +    VIR_FREE(key);
> +    return ret;
> +}
> +
>  /* Increase ref count if the entry already exists, otherwise
>   * add a new entry.
>   */
> @@ -840,6 +903,9 @@ qemuAddSharedDisk(virQEMUDriverPtr driver,
>          !disk->shared || !disk->src)
>          return 0;
>  
> +    if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
> +        return -1;
> +
>      if (!(key = qemuGetSharedDiskKey(disk->src)))
>          goto cleanup;
>  
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f98bd9b..86f9674 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5825,11 +5825,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>          goto end;
>      }
>  
> -    if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> -        disk->shared &&
> -        (qemuCheckSharedDisk(driver, disk) < 0))
> -        goto end;
> -

There's a lot of work going on in this module that all becomes for
naught if it's not a shared disk.  In fact if we fail to add the disk in
this module, I also note that this code path would still call
qemuSetUnprivSGIO() (in the live case, if AddShared) - so if it's not a
sharable disk, then we're setting something we may expect to set, right?

>      if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
>          goto end;
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 8425cbb..6ffb680 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3556,56 +3556,6 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
>      return 0;
>  }
>  
> -/* Check if a shared disk's setting conflicts with the conf
> - * used by other domain(s). Currently only checks the sgio
> - * setting. Note that this should only be called for disk with
> - * block source.
> - *
> - * Returns 0 if no conflicts, otherwise returns -1.
> - */
> -int
> -qemuCheckSharedDisk(virQEMUDriverPtr driver,
> -                    virDomainDiskDefPtr disk)
> -{
> -    int val;
> -    size_t *ref = NULL;
> -    char *key = NULL;
> -    int ret = 0;
> -
> -    if (!(key = qemuGetSharedDiskKey(disk->src)))
> -        return -1;
> -
> -    /* It can't be conflict if no other domain is
> -     * is sharing it.
> -     */
> -    if (!(ref = virHashLookup(driver->sharedDisks, key)))
> -        goto cleanup;
> -
> -    if (ref == (void *)0x1)
> -        goto cleanup;
> -
> -    if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
> -        ret = -1;
> -        goto cleanup;
> -    }
> -
> -    if ((val == 0 &&
> -         (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
> -          disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
> -        (val == 1 &&
> -         disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
> -        goto cleanup;
> -
> -    virReportError(VIR_ERR_OPERATION_INVALID,
> -                   _("sgio of shared disk '%s' conflicts with other "
> -                     "active domains"), disk->src);
> -    ret = -1;
> -
> -cleanup:
> -    VIR_FREE(key);
> -    return ret;
> -}
> -
>  int qemuProcessStart(virConnectPtr conn,
>                       virQEMUDriverPtr driver,
>                       virDomainObjPtr vm,
> @@ -3962,9 +3912,6 @@ int qemuProcessStart(virConnectPtr conn,
>          if (qemuAddSharedDisk(driver, disk) < 0)
>              goto cleanup;
>  
> -        if (qemuCheckSharedDisk(driver, disk) < 0)
> -            goto cleanup;
> -
>          if (qemuSetUnprivSGIO(disk) < 0)
>              goto cleanup;
>      }
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index cbdab24..7c620d4 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -100,7 +100,4 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver,
>                                 virBitmapPtr nodemask);
>  int qemuSetUnprivSGIO(virDomainDiskDefPtr disk);
>  
> -int qemuCheckSharedDisk(virQEMUDriverPtr driver,
> -                        virDomainDiskDefPtr disk);
> -
>  #endif /* __QEMU_PROCESS_H__ */
>