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

[Osier Yang]

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.

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

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.
>