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

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



On 08/05/13 08:05, John Ferlan wrote:
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


To not introduce coverity errors anymore, I setup the coverity
env on my local box, and the errors in this patch are disapeared
after add the else branch:
} else {
    return 0;
}

So pushed. Thanks.

Osier


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