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

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.

> +int > +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

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.