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

Re: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains



On 16/01/14 08:51, John Ferlan wrote:

On 01/08/2014 09:51 AM, Osier Yang wrote:
It doesn't make sense to fail if the SCSI host device is specified
as "shareable" explicitly between domains (NB, it works if and only
if the device is specified as "shareable" for *all* domains,
otherwise it fails).

To fix the problem, this patch introduces an array for virSCSIDevice
struct, which records all the names of domain which are using the
device (note that the recorded domains must specifiy the device as
shareable).  And the change on the data struct brings on many
subsequent changes in the code.

* src/util/virscsi.h:
   - Remove virSCSIDeviceGetUsedBy
   - Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel
   - Add virSCSIDeviceIsAvailable

* src/util/virscsi.c:
   - struct virSCSIDevice: Change "used_by" to be an array; Add
     "n_used_by" as the array count
   - virSCSIDeviceGetUsedBy: Removed
   - virSCSIDeviceFree: frees the "used_by" array
   - virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential
     memory corruption
   - virSCSIDeviceIsAvailable: New
   - virSCSIDeviceListDel: Change the logic, for device which is already
     in the list, just remove the corresponding entry in "used_by"
   - Copyright updating

* src/libvirt_private.sys:
   - virSCSIDeviceGetUsedBy: Remove
   - virSCSIDeviceIsAvailable: New

* src/qemu/qemu_hostdev.c:
   - qemuUpdateActiveScsiHostdevs: Check if the device existing before
     adding it to the list;
   - qemuPrepareHostdevSCSIDevices: Error out if the not all domains
     use the device as "shareable"; Also don't try to add the device
     to the activeScsiHostdevs list if it already there.
   - qemuDomainReAttachHostScsiDevices: Change the logic according
     to the changes on helpers.
---
  src/libvirt_private.syms |  2 +-
  src/qemu/qemu_hostdev.c  | 75 ++++++++++++++++++++++++++----------------------
  src/util/virscsi.c       | 48 +++++++++++++++++++++++++------
  src/util/virscsi.h       |  7 +++--
  4 files changed, 84 insertions(+), 48 deletions(-)

......

      virObjectUnlock(driver->activeScsiHostdevs);
@@ -1380,8 +1390,7 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
      virObjectLock(driver->activeScsiHostdevs);
      for (i = 0; i < nhostdevs; i++) {
          virDomainHostdevDefPtr hostdev = hostdevs[i];
-        virSCSIDevicePtr scsi, tmp;
-        const char *used_by = NULL;
+        virSCSIDevicePtr scsi;
          virDomainDeviceDef dev;
dev.type = VIR_DOMAIN_DEVICE_HOSTDEV;
@@ -1411,30 +1420,26 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
          /* Only delete the devices which are marked as being used by @name,
           * because qemuProcessStart could fail on the half way. */
- tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi);
-        virSCSIDeviceFree(scsi);
-
-        if (!tmp) {
+        if (!virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi)) {
I think you should keep (and perhaps rename) the tmp pointer and then
pass it to virSCSIDeviceListDel() since that function will call the the
find again anyway

              VIR_WARN("Unable to find device %s:%d:%d:%d "
                       "in list of active SCSI devices",
                       hostdev->source.subsys.u.scsi.adapter,
                       hostdev->source.subsys.u.scsi.bus,
                       hostdev->source.subsys.u.scsi.target,
                       hostdev->source.subsys.u.scsi.unit);
+            virSCSIDeviceFree(scsi);
              continue;
          }
- used_by = virSCSIDeviceGetUsedBy(tmp);
-        if (STREQ_NULLABLE(used_by, name)) {
-            VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs",
-                      hostdev->source.subsys.u.scsi.adapter,
-                      hostdev->source.subsys.u.scsi.bus,
-                      hostdev->source.subsys.u.scsi.target,
-                      hostdev->source.subsys.u.scsi.unit,
-                      name);
+        VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs",
+                   hostdev->source.subsys.u.scsi.adapter,
+                   hostdev->source.subsys.u.scsi.bus,
+                   hostdev->source.subsys.u.scsi.target,
+                   hostdev->source.subsys.u.scsi.unit,
+                   name);
- virSCSIDeviceListDel(driver->activeScsiHostdevs, tmp);
-        }
+        virSCSIDeviceListDel(driver->activeScsiHostdevs, scsi, name);
+        virSCSIDeviceFree(scsi);
      }
      virObjectUnlock(driver->activeScsiHostdevs);
  }
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 3998c3a..42030c5 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -1,6 +1,7 @@
  /*
   * virscsi.c: helper APIs for managing host SCSI devices
   *
+ * Copyright (C) 2013 - 2014 Red Hat, Inc.
   * Copyright (C) 2013 Fujitsu, Inc.
   *
   * This library is free software; you can redistribute it and/or
@@ -55,7 +56,8 @@ struct _virSCSIDevice {
      char *name; /* adapter:bus:target:unit */
      char *id;   /* model:vendor */
      char *sg_path; /* e.g. /dev/sg2 */
-    const char *used_by; /* name of the domain using this dev */
+    char **used_by; /* name of the domains using this dev */
+    size_t n_used_by; /* how many domains are using this dev */
bool readonly;
      bool shareable;
@@ -256,26 +258,37 @@ cleanup:
  void
  virSCSIDeviceFree(virSCSIDevicePtr dev)
  {
+    size_t i;
+
      if (!dev)
          return;
VIR_FREE(dev->id);
      VIR_FREE(dev->name);
      VIR_FREE(dev->sg_path);
+    for (i = 0; i < dev->n_used_by; i++) {
+        VIR_FREE(dev->used_by[i]);
+    }
+    VIR_FREE(dev->used_by);
      VIR_FREE(dev);
  }
-void
+int
  virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev,
                         const char *name)
  {
-    dev->used_by = name;
+    char *copy = NULL;
+
+    if (VIR_STRDUP(copy, name) < 0)
+        return -1;
+
+    return VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy);
  }
-const char *
-virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev)
+bool
+virSCSIDeviceIsAvailable(virSCSIDevicePtr dev)
  {
-    return dev->used_by;
+    return dev->n_used_by == 0;
  }
const char *
@@ -406,10 +419,27 @@ virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
void
  virSCSIDeviceListDel(virSCSIDeviceListPtr list,
-                     virSCSIDevicePtr dev)
+                     virSCSIDevicePtr dev,
+                     const char *name)
  {
-    virSCSIDevicePtr ret = virSCSIDeviceListSteal(list, dev);
-    virSCSIDeviceFree(ret);
+    virSCSIDevicePtr ret = NULL;
+    size_t i;
+
+    if (!(ret = virSCSIDeviceListFind(list, dev)))
+        return;
since there's only one caller and it already did a
virSCSIDeviceListFind() (but threw away the results) - couldn't the
caller save and pass the results rather than making the same call twice?

I don't think a V3 would be necessary based on your thoughts.  Again -
this is something for post 1.2.1


Agreed, I will change when pushing later.  Thanks for the reviewing.

Osier


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