[libvirt] [PATCH 25/25] qemu: Check conflicts for shared scsi host device

[John Ferlan]

On 05/03/2013 02:07 PM, Osier Yang wrote:
> Just like previous patches, this changes qemuCheckSharedDisk
> into qemuCheckSharedDevice, which takes a virDomainDeviceDefPtr
> argument instead.
> ---
>  src/qemu/qemu_conf.c | 86 +++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 61 insertions(+), 25 deletions(-)
> 

Ahhh finally - never thought I'd get to the last one :-)  Taken longer
than I wanted!

> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index cf1c7ee..f8264f6 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -943,29 +943,55 @@ qemuGetSharedDeviceKey(const char *device_path)
>      return key;
>  }
>  
> -/* Check if a shared disk's setting conflicts with the conf
> +/* Check if a shared device'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.
> + * block source if the device type is disk.
>   *
>   * Returns 0 if no conflicts, otherwise returns -1.
>   */
>  static int
> -qemuCheckSharedDisk(virHashTablePtr sharedDevices,
> -                    virDomainDiskDefPtr disk)
> +qemuCheckSharedDevice(virHashTablePtr sharedDevices,
> +                      virDomainDeviceDefPtr dev)
>  {
> +    virDomainDiskDefPtr disk = NULL;
> +    virDomainHostdevDefPtr hostdev = NULL;
>      char *sysfs_path = NULL;
>      char *key = NULL;
> +    char *hostdev_name = NULL;
> +    char *hostdev_path = NULL;
> +    char *device_path = NULL;
>      int val;
>      int ret = 0;
>  
> -    /* The only conflicts between shared disk we care about now
> -     * is sgio setting, which is only valid for device='lun'.
> -     */
> -    if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN)
> -        return 0;

Coverity note #1:

(2) Event cond_false: 	Condition "dev->type == VIR_DOMAIN_DEVICE_DISK",
taking false branch


> +    if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> +        disk = dev->data.disk;
> +
> +        /* The only conflicts between shared disk we care about now
> +         * is sgio setting, which is only valid for device='lun'.
> +         */
> +        if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN)
> +            return 0;
> +
> +        device_path = disk->src;

Coverity note #2:

(3) Event cond_false: 	Condition "dev->type ==
VIR_DOMAIN_DEVICE_HOSTDEV", taking false branch

> +    } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
> +        hostdev = dev->data.hostdev;
> +
> +        if (!(hostdev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter,
> +                                                     hostdev->source.subsys.u.scsi.bus,
> +                                                     hostdev->source.subsys.u.scsi.target,
> +                                                     hostdev->source.subsys.u.scsi.unit)))
> +            goto cleanup;
> +
> +        if (virAsprintf(&hostdev_path, "/dev/%s", hostdev_name) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +
> +        device_path = hostdev_path;
> +    }

Coverity Note #3

In the "else" condition (not here) - that means "device_path = NULL"
which is going to be a problem shortly....

Should we return 0 as an "else" condition?


>  
> -    if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL))) {
> +    if (!(sysfs_path = virGetUnprivSGIOSysfsPath(device_path, NULL))) {
>          ret = -1;
>          goto cleanup;
>      }
> @@ -976,7 +1002,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices,
>      if (!virFileExists(sysfs_path))
>          goto cleanup;
>  

Coverity complains:

(8) Event var_deref_model: 	Passing null pointer "device_path" to
function "qemuGetSharedDeviceKey(char const *)", which dereferences it.
(The dereference is assumed on the basis of the 'nonnull' parameter
attribute.)
Also see events: 	[assign_zero]


Fix that and it's an ACK


John
> -    if (!(key = qemuGetSharedDeviceKey(disk->src))) {
> +    if (!(key = qemuGetSharedDeviceKey(device_path))) {
>          ret = -1;
>          goto cleanup;
>      }
> @@ -987,7 +1013,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices,
>      if (!(virHashLookup(sharedDevices, key)))
>          goto cleanup;
>  
> -    if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
> +    if (virGetDeviceUnprivSGIO(device_path, NULL, &val) < 0) {
>          ret = -1;
>          goto cleanup;
>      }
> @@ -999,26 +1025,36 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices,
>           disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED))
>          goto cleanup;
>  
> -    if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
> -        virReportError(VIR_ERR_OPERATION_INVALID,
> -                       _("sgio of shared disk 'pool=%s' 'volume=%s' conflicts "
> -                         "with other active domains"),
> -                       disk->srcpool->pool,
> -                       disk->srcpool->volume);
> +    if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> +        if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
> +            virReportError(VIR_ERR_OPERATION_INVALID,
> +                           _("sgio of shared disk 'pool=%s' 'volume=%s' conflicts "
> +                             "with other active domains"),
> +                           disk->srcpool->pool,
> +                           disk->srcpool->volume);
> +        } else {
> +            virReportError(VIR_ERR_OPERATION_INVALID,
> +                           _("sgio of shared disk '%s' conflicts with other "
> +                             "active domains"), disk->src);
> +        }
>      } else {
>          virReportError(VIR_ERR_OPERATION_INVALID,
> -                       _("sgio of shared disk '%s' conflicts with other "
> -                         "active domains"), disk->src);
> +                       _("sgio of shared scsi host device '%s-%d-%d-%d' conflicts "
> +                          "with other active domains"),
> +                       hostdev->source.subsys.u.scsi.adapter,
> +                       hostdev->source.subsys.u.scsi.bus,
> +                       hostdev->source.subsys.u.scsi.target,
> +                       hostdev->source.subsys.u.scsi.unit);
>      }
>  
>      ret = -1;
> -
>  cleanup:
> +    VIR_FREE(hostdev_name);
> +    VIR_FREE(hostdev_path);
>      VIR_FREE(sysfs_path);
>      VIR_FREE(key);
>      return ret;
>  }
> -
>  bool
>  qemuSharedDeviceEntryDomainExists(qemuSharedDeviceEntryPtr entry,
>                                    const char *name,
> @@ -1133,10 +1169,10 @@ qemuAddSharedDevice(virQEMUDriverPtr driver,
>      }
>  
>      qemuDriverLock(driver);
> -    if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> -        if (qemuCheckSharedDisk(driver->sharedDevices, disk) < 0)
> -            goto cleanup;
> +    if (qemuCheckSharedDevice(driver->sharedDevices, dev) < 0)
> +        goto cleanup;
>  
> +    if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
>          if (!(key = qemuGetSharedDeviceKey(disk->src)))
>              goto cleanup;
>      } else {
>