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

Re: [libvirt] [PATCH] Refresh QEMU driver caps in getCapabilities



Daniel P. Berrange wrote:
> On Tue, Apr 28, 2009 at 12:25:41PM -0400, Cole Robinson wrote:
>> Hi all,
>>
>> The attached patch fixes QEMU getCapabilities to refresh caps before
>> returning them to the user. Currently the capabilities are only
>> refreshed once (at daemon startup), which means libvirtd needs to be
>> restarted to pick up changes if QEMU or KVM are installed while the
>> daemon is running. See:
>> https://bugzilla.redhat.com/show_bug.cgi?id=460649
>>
>> There are several things 'wrong' with this change:
>>
>> - We reset/rescan fields that won't change (host arch, mac address
>> prefix). This should be fixed at some point, but isn't a big deal since
>> total performance impact is negligible (see below).
>>
>> - We only refresh the capabilities when the user calls getCapabilities,
>> which means we are still carrying around stale caps prior to that, which
>> is what libvirt validates against. In practice, virt-manager and
>> virt-install both call getCapabilities often so this isn't an issue,
>> though the caps internal API should probably address this at some point.
>>
>> To test the performance impact, I used a simple python script:
>>
>> import libvirt
>> conn = libvirt.open("qemu:///system")
>> for i in range(0, 30):
>>     conn.getCapabilities()
>>
>> The time difference was on average .02 seconds slower, which I think is
>> negligible.
> 
> More importantly I don't see this API call as being a performance
> critical one. It is not something applicaitions will be  calling 
> inside a hot loop.
> 
> 
>> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
>> index 79ee072..6b5c17f 100644
>> --- a/src/qemu_driver.c
>> +++ b/src/qemu_driver.c
>> @@ -1872,10 +1872,12 @@ static int qemudGetNodeInfo(virConnectPtr conn,
>>  
>>  static char *qemudGetCapabilities(virConnectPtr conn) {
>>      struct qemud_driver *driver = conn->privateData;
>> -    char *xml;
>> +    char *xml = NULL;
>>  
>>      qemuDriverLock(driver);
>> -    if ((xml = virCapabilitiesFormatXML(driver->caps)) == NULL)
>> +    virCapabilitiesFree(qemu_driver->caps);
>> +    if ((qemu_driver->caps = qemudCapsInit()) == NULL ||
>> +        (xml = virCapabilitiesFormatXML(driver->caps)) == NULL)
>>          virReportOOMError(conn);
>>      qemuDriverUnlock(driver);
> 
> The thing to be wary of now, is that all use of driver->caps needs
> to be protected by the driver mutex. Most usages are OK, but I
> spotted a couple that are not.
> 
> Daniel

Ahh, yes I didn't think of that repercussion. I'll update this patch.

Thanks,
Cole


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