[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日 19:05, Jiri Denemark wrote:
On Tue, Dec 11, 2012 at 21:37:18 +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,
* 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
  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.
+ */
+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;
+    }

In addition to Daniel's comments, I think this hash table should contain
device number rather than disk path since /dev is full of symlinks. One
domain may be configured to use /dev/sdg while another one would use
/dev/disk/by-id/..., another one /dev/disk/by-uuid/... and they can all
lead to the same block device. Thus you need to canonicalize disk_path
first, e.g., into major:minor and store that in the hash table.

Hum, agreed, this is really good catch which saves us from the
potential bug. Thanks.


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