[libvirt] [PATCH] qemu: Avoid holding the driver lock in trivial snapshot API's

[Peter Krempa]

On 09/24/12 18:51, Eric Blake wrote:
> On 09/24/2012 06:57 AM, Peter Krempa wrote:
>> In most of the snapshot API's there's no need to hold the driver lock
>> the whole time.
>>
>> This patch adds helper functions that get the domain object in functions
>> that don't require the driver lock and simplifies call paths from
>> snapshot-related API's.
>> ---
>>   src/conf/domain_conf.c |   3 +-
>>   src/qemu/qemu_driver.c | 306 +++++++++++++++----------------------------------
>>   2 files changed, 94 insertions(+), 215 deletions(-)
>>
>
>>   void virDomainObjUnlock(virDomainObjPtr obj)
>>   {
>> -    virMutexUnlock(&obj->lock);
>> +    if (obj)
>> +        virMutexUnlock(&obj->lock);
>>   }
>
> I agree with Daniel that this hunk is bad.  But most of the patch is
> worthwhile...

Ok, I'll get rid of this.

>
>> +/* Looks up the domain obj and unlocks the driver */
>> +static virDomainObjPtr
>> +qemuDomObjFromDomain(virDomainPtr domain)
>> +{
>> +    struct qemud_driver *driver = domain->conn->privateData;
>> +    virDomainObjPtr vm;
>> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
>> +
>> +    qemuDriverLock(driver);
>> +    vm = virDomainFindByUUID(&driver->domains, domain->uuid);
>> +    qemuDriverUnlock(driver);
>> +    if (!vm) {
>> +        virUUIDFormat(domain->uuid, uuidstr);
>> +        virReportError(VIR_ERR_NO_DOMAIN,
>> +                       _("no domain with matching uuid '%s'"), uuidstr);
>> +    }
>> +
>> +    return vm;
>> +}
>
> This is a nice function for one-shot lookups, when the rest of the
> function does not need the driver locked.
>
>
>> @@ -11305,59 +11359,39 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names,
>>                                          int nameslen,
>>                                          unsigned int flags)
>>   {
>> -    struct qemud_driver *driver = domain->conn->privateData;
>>       virDomainObjPtr vm = NULL;
>>       int n = -1;
>>
>>       virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS |
>>                     VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1);
>>
>> -    qemuDriverLock(driver);
>> -    vm = virDomainFindByUUID(&driver->domains, domain->uuid);
>> -    if (!vm) {
>> -        char uuidstr[VIR_UUID_STRING_BUFLEN];
>> -        virUUIDFormat(domain->uuid, uuidstr);
>> -        virReportError(VIR_ERR_NO_DOMAIN,
>> -                       _("no domain with matching uuid '%s'"), uuidstr);
>> +    if (!(vm = qemuDomObjFromDomain(domain)))
>>           goto cleanup;
>> -    }
>
> And this is a nice usage of the new helpers (probably a LOT more APIs
> that can use them, rather than just snapshots).

Yes, there's a lot of API's that don't need the driver locked. I just 
chose a subset so this patch doesn't grow out of control and is hard to 
review. I also wanted to establish a pattern with this that we can use 
later on, as the usual way to add new API's is to copy something existing.

>
>>
>>       n = virDomainSnapshotObjListGetNames(vm->snapshots, NULL, names, nameslen,
>>                                            flags);
>>
>>   cleanup:
>> -    if (vm)
>> -        virDomainObjUnlock(vm);
>> -    qemuDriverUnlock(driver);
>> +    virDomainObjUnlock(vm);
>>       return n;
>
> This hunk, however, should be:
>
> cleanup:
>      if (vm)
>          virDomainObjUnlock(vm);
>      return n;
>
> Looking forward to v2.
>

Posting v2 now.