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

Re: [libvirt] [PATCH 01/12] qemu: Add a hash table for the shared disks



On 2012年12月13日 01:09, Daniel P. Berrange wrote:
On Tue, Dec 11, 2012 at 09:37:18PM +0800, Osier Yang wrote:
This introduces a hash table for qemu driver, to store the shared
disk's info as (@disk_path, { ref_count, @orig_cdbfilter}). @ref_count
is the number of domains which shares the disk. @orig_cdbfilter is
the original cdbfilter setting of the shared disk, it will be used
to restore the the disk's cdbfilter setting to original value by
later patches.

* src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type
                          virHashTablePtr; New struct qemuSharedDiskEntry;
                          Declare helpers qemuAddSharedDisk,
                          qemuRemoveSharedDisk)
* src/qemu/qemu_conf.c (Implement the two helpers)
* src/qemu/qemu_process.c (Update 'sharedDisks' when domain
                            starting and shutdown)
* src/qemu/qemu_driver.c (Update 'sharedDisks' when attaching
                           or detaching disk).

0 is passed for orig_cdbfilter temporarily, later patches will update
it.
---
  src/qemu/qemu_conf.c    |   48 +++++++++++++++++++++++++++++++++++++++++++++++
  src/qemu/qemu_conf.h    |   18 +++++++++++++++++
  src/qemu/qemu_driver.c  |   17 ++++++++++++++++
  src/qemu/qemu_process.c |   17 +++++++++++++++-
  4 files changed, 99 insertions(+), 1 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 8d380a1..2b21186 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -556,3 +556,51 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,

      virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun,&data);
  }
+
+/* Increase ref count if the entry already exists, otherwise
+ * add a new entry.
+ */
+int
+qemuAddSharedDisk(virHashTablePtr sharedDisks,
+                  const char *disk_path,
+                  int orig_cdbfilter)
+{
+    qemuSharedDiskEntryPtr entry = NULL;
+
+    if ((entry = virHashLookup(sharedDisks, disk_path))) {
+        entry->ref++;
+    } else {
+        if (VIR_ALLOC(entry)<  0)
+            return -1;
+
+        entry->ref = 1;
+        entry->orig_cdbfilter = orig_cdbfilter;
+
+        if (virHashAddEntry(sharedDisks, disk_path, entry))
+            return -1;

Leaking 'entry' in failure path.

Okay, it's missed when changing the hash value from
only 'ref' count to a struct containing the original
cdbfilter value.


@@ -6035,6 +6039,12 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
              VIR_WARN("Failed to teardown cgroup for disk path %s",
                       NULLSTR(disk->src));
      }
+
+    if (ret == 0&&  disk->shared) {
+        if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0)<  0)
+            VIR_WARN("Failed to add disk '%s' to shared disk table",
+                     disk->src);
+    }

You are not allowed to reference 'disk->src' unless you've validate
disk->type is FILE or BLOCK.

We only need to track this for TYPE_BLOCK in any case

Oh, right, okay.



  end:
      if (cgroup)
          virCgroupFree(&cgroup);
@@ -6149,6 +6159,13 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
                         virDomainDiskDeviceTypeToString(disk->type));
          break;
      }
+
+    if (ret == 0&&  disk->shared) {
+        if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src)<  0)
+             VIR_WARN("Failed to remove disk '%s' from shared disk table",
+                      disk->src);
+    }

Same crash problem here


+
      return ret;
  }

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ab04599..89152b8 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3706,8 +3706,15 @@ int qemuProcessStart(virConnectPtr conn,

      /* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */
      for (i = 0; i<  vm->def->ndisks; i++) {
-        if (vm->def->disks[i]->rawio == 1)
+        virDomainDiskDefPtr disk = vm->def->disks[i];
+
+        if (disk->rawio == 1)
              virCommandAllowCap(cmd, CAP_SYS_RAWIO);
+
+        if (disk->shared) {
+            if (qemuAddSharedDisk(driver->sharedDisks, disk->src, 0)<  0)
+                goto cleanup;
+        }


And here

      }

      virCommandSetPreExecHook(cmd, qemuProcessHook,&hookData);
@@ -4104,6 +4111,14 @@ void qemuProcessStop(virQEMUDriverPtr driver,
                                            flags&  VIR_QEMU_PROCESS_STOP_MIGRATED);
      virSecurityManagerReleaseLabel(driver->securityManager, vm->def);

+    for (i = 0; i<  vm->def->ndisks; i++) {
+        virDomainDiskDefPtr disk = vm->def->disks[i];
+
+        if (disk->shared) {
+            ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src));
+        }
+    }

And here

Daniel


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