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

[Eric Blake]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130103/8f9023ab/attachment-0001.sig>