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

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

On 2013年01月04日 04:02, Eric Blake wrote:
On 01/02/2013 07:37 AM, Osier Yang wrote:
This introduces a hash table for qemu driver, to store the shared
disk's info as (@major:minor, @ref_count). @ref_count is the number
of domains which shares the disk.

Since we only care about if the disk support unprivileged SG_IO
commands, and the SG_IO commands only make sense for block disk,
this patch only manages (add/remove hash entry) the shared disk for
block disk.

* src/qemu/qemu_conf.h: (Add member 'sharedDisks' of type
                          virHashTablePtr; Declare helpers
                          qemuGetSharedDiskKey, qemuAddSharedDisk
                          and qemuRemoveSharedDisk)
* src/qemu/qemu_conf.c (Implement the 3 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).
  src/qemu/qemu_conf.c    |   86 +++++++++++++++++++++++++++++++++++++++++++++++
  src/qemu/qemu_conf.h    |   12 ++++++
  src/qemu/qemu_driver.c  |   22 ++++++++++++
  src/qemu/qemu_process.c |   15 ++++++++
  4 files changed, 135 insertions(+), 0 deletions(-)

ACK, as I'd like to see this go in sooner rather than later, and what
you have works.  However...

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index c6deb10..8440247 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -552,3 +552,89 @@ qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,

      virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun,&data);
+/* Construct the hash key for sharedDisks as "major:minor" */
+char *
+qemuGetSharedDiskKey(const char *disk_path)

Why are we converting major and minor into a malloc'd char*,...

+    int major, minor;

Here, I'd use 'maj' and 'min' to avoid any shadowing of the major() and
minor() macros.

Oh, okay, I missed that.

+qemuAddSharedDisk(virHashTablePtr sharedDisks,
+                  const char *disk_path)
+    size_t *ref = NULL;
+    char *key = NULL;
+    if (!(key = qemuGetSharedDiskKey(disk_path)))
+        return -1;
+    if ((ref = virHashLookup(sharedDisks, key))) {

...when you could just use a single int value comprising the combination
of major and minor as the hash key, if only you had used

Sounds good, how about something like "805516", where "8" is major, and "16" is the minor? It should be small enough to be not overflowing.

virHashCreateFull() with custom comparators.  That would be more
efficient (no need to malloc each kay, comparisons are much faster as an
integer compare instead of a strcmp, and so on).  It may be worth a
followup patch that re-does the hash table to be more efficient.

+++ b/src/qemu/qemu_driver.c
@@ -843,6 +843,9 @@ qemuStartup(bool privileged,
      if ((qemu_driver->inactivePciHostdevs = pciDeviceListNew()) == NULL)
          goto error;

+    if (!(qemu_driver->sharedDisks = virHashCreate(30, NULL)))

Here's where the virHashCreateFull would let you avoid having to convert
the key into a string.

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