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

Re: [libvirt] [PATCH 1/4] qemu: Add checking in helpers for sgio setting



On 02/08/2013 08:07 AM, Osier Yang wrote:
> This moves the checking into the helpers, to avoid the
> callers missing the checking.
> ---
>  src/qemu/qemu_conf.c    |   20 ++++++++++++++++----
>  src/qemu/qemu_conf.h    |    4 ++--
>  src/qemu/qemu_driver.c  |   18 +++++++-----------
>  src/qemu/qemu_process.c |   21 ++++++++++++---------
>  4 files changed, 37 insertions(+), 26 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 17f7d45..69ba710 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -726,12 +726,20 @@ qemuGetSharedDiskKey(const char *disk_path)
>   */
>  int
>  qemuAddSharedDisk(virHashTablePtr sharedDisks,
> -                  const char *disk_path)
> +                  virDomainDiskDefPtr disk)
>  {
>      size_t *ref = NULL;
>      char *key = NULL;
>  
> -    if (!(key = qemuGetSharedDiskKey(disk_path)))
> +    /* Currently the only conflicts we have to care about
> +     * for the shared disk is "sgio" setting, which is only
> +     * valid for block disk.
> +     */
> +    if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
> +        !disk->shared || !disk->src)
> +        return 0;
> +
> +    if (!(key = qemuGetSharedDiskKey(disk->src)))
>          return -1;
>  
>      if ((ref = virHashLookup(sharedDisks, key))) {
> @@ -755,12 +763,16 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
>   */
>  int
>  qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
> -                     const char *disk_path)
> +                     virDomainDiskDefPtr disk)
>  {
>      size_t *ref = NULL;
>      char *key = NULL;
>  
> -    if (!(key = qemuGetSharedDiskKey(disk_path)))
> +    if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
> +        !disk->shared || !disk->src)
> +        return 0;
> +
> +    if (!(key = qemuGetSharedDiskKey(disk->src)))
>          return -1;
>  
>      if (!(ref = virHashLookup(sharedDisks, key))) {
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 60c4109..8e84bcf 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -270,11 +270,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
>                                     virConnectPtr conn);
>  
>  int qemuAddSharedDisk(virHashTablePtr sharedDisks,
> -                      const char *disk_path)
> +                      virDomainDiskDefPtr disk)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
>  int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
> -                         const char *disk_path)
> +                         virDomainDiskDefPtr disk)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  char * qemuGetSharedDiskKey(const char *disk_path)
>      ATTRIBUTE_NONNULL(1);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 979a027..043efd3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5855,11 +5855,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>      }
>  
>      if (ret == 0) {
> -        if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) {
> -            if (qemuAddSharedDisk(driver->sharedDisks, disk->src) < 0)
> -                VIR_WARN("Failed to add disk '%s' to shared disk table",
> -                         disk->src);
> -        }
> +        if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
> +            VIR_WARN("Failed to add disk '%s' to shared disk table",
> +                     disk->src);
>  
>          if (qemuSetUnprivSGIO(disk) < 0)
>              VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src);

Does there need to be a NULLSTR(disk->src)?  Doesn't seem so, but just
checking.  I only note this because the qemuAddSharedDisk() has a
!disk->src check prior to returning zero.

> @@ -5980,12 +5978,10 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
>          break;
>      }
>  
> -    if (ret == 0 &&
> -        disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> -        disk->shared) {
> -        if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src) < 0)
> -             VIR_WARN("Failed to remove disk '%s' from shared disk table",
> -                      disk->src);
> +    if (ret == 0) {
> +        if (qemuRemoveSharedDisk(driver->sharedDisks, disk) < 0)
> +            VIR_WARN("Failed to remove disk '%s' from shared disk table",
> +                     disk->src);

Similar comment here w/r/t NULLSTR and checks in Remove code

>      }
>  
>      return ret;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 30f923a..bc171a4 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3453,6 +3453,13 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
>  {
>      int val = -1;
>  
> +    /* "sgio" is only valid for block disk; cdrom
> +     * and floopy disk can have empty source.
> +     */
> +    if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
> +        !disk->src)
> +        return 0;
> +
>      if (disk->sgio)
>          val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED);
>  
> @@ -3875,13 +3882,11 @@ int qemuProcessStart(virConnectPtr conn,
>                             _("Raw I/O is not supported on this platform"));
>  #endif
>  
> -        if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) {
> -            if (qemuAddSharedDisk(driver->sharedDisks, disk->src) < 0)
> -                goto cleanup;
> +        if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
> +            goto cleanup;
>  
> -            if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
> -                goto cleanup;
> -        }
> +        if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
> +            goto cleanup;
>  
>          if (qemuSetUnprivSGIO(disk) < 0)
>              goto cleanup;
> @@ -4288,9 +4293,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>      for (i = 0; i < vm->def->ndisks; i++) {
>          virDomainDiskDefPtr disk = vm->def->disks[i];
>  
> -        if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) {
> -            ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src));
> -        }
> +        ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
>      }
>  
>      /* Clear out dynamically assigned labels */
> 

ACK - everything seems straightforward to me.

John


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