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

[John Ferlan]

On 02/08/2013 08:08 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    |   50 ++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_driver.c  |    5 ----
>  src/qemu/qemu_process.c |   53 -----------------------------------------------
>  src/qemu/qemu_process.h |    3 --
>  4 files changed, 50 insertions(+), 61 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 69ba710..3eeea4b 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -721,6 +721,53 @@ 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 *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(sharedDisks, key)))
> +        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;
> +}
> +
>  /* Increase ref count if the entry already exists, otherwise
>   * add a new entry.
>   */
> @@ -739,6 +786,9 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
>          !disk->shared || !disk->src)
>          return 0;
>  
> +    if (qemuCheckSharedDisk(sharedDisks, disk) < 0)
> +        return -1;
> +

I take the order previously in qemuProcessStart() was wrong then - it
was adding it first, then checking if it was shared.  The DiskLive code
checked shared first, then added, which does seem more correct.

However, the change is subtle enough to make me wonder about the reason
for the ordering of calls in the DiskLive code that would check shared
before calls to qemuDomainDetermineDiskChain() and qemuSetupDiskCgroup()

Seems as though now the code in DiskLive may need some extra backout
code if the add now fails because of the check


>      if (!(key = qemuGetSharedDiskKey(disk->src)))
>          return -1;
>  
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 043efd3..1dc9789 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5798,11 +5798,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>          goto end;
>      }
>  
> -    if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> -        disk->shared &&
> -        (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0))
> -        goto end;
> -
>      if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
>          goto end;
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index bc171a4..1e0876c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3483,56 +3483,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(virHashTablePtr sharedDisks,
> -                    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(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,
> @@ -3885,9 +3835,6 @@ int qemuProcessStart(virConnectPtr conn,
>          if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
>              goto cleanup;
>  
> -        if (qemuCheckSharedDisk(driver->sharedDisks, 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 2dc8041..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(virHashTablePtr sharedDisks,
> -                        virDomainDiskDefPtr disk);
> -
>  #endif /* __QEMU_PROCESS_H__ */
> 

Need to understand ramifications of change before ACK I think. Unless
someone else comes in after me with a deeper understanding and says the
order is fine.

John