[libvirt] [PATCH] qemu: qemuDomainRename and virDomainObjListNumOfDomains ABBA deadlock fix

[Martin Kletzander]

On Tue, Jan 26, 2016 at 02:14:23PM +0100, Jiri Denemark wrote:
>On Tue, Jan 26, 2016 at 13:26:32 +0100, Michal Privoznik wrote:
>> On 25.01.2016 10:16, Maxim Nestratov wrote:
>...
>> >
>> >      virCheckFlags(0, ret);
>> >
>> > -    if (!(vm = qemuDomObjFromDomain(dom)))
>> > +    virObjectLock(driver->domains);
>>
>> This is rather ugly. While driver->domains is a lockable object, it's
>> internals are intentionally hidden from the rest of the code so that
>> nobody from outside should touch them. That's why we have locking APIs
>> over virDomainObjList object. I know this will work, I just find it hackish.
>
>Yeah, I don't like this either.
>
>> However, I find your approach understandable and kind of agree with it.
>> What we should do is:
>>
>> 1) lock list
>> 2) lookup domain
>> 3) begin MODIFY job on the domain (*)
>>
>> 4) rename the domain (here are hidden all the checks, moving the XML
>> file, etc. - basically everything from the qemuDomainRename())
>>
>> 5) end job
>> 6) unlock domain
>> 7) unlock list
>>
>> * - here we will need BeginJob() without timeout - we don't want to keep
>> the list locked longer than needed. Either setting the job is done
>> instantly or not at all.
>
>This sounds very ugly and fragile too.
>
>> What if, instead of introducing bunch of Locked() functions we introduce
>> a new internal API to virDomainObjList that will look like this:
>>
>> int virDomainObjListRename(virDomainObjListPtr domains,
>>                            virDomainObjPtr dom,
>>                            const char *new_name,
>>                            int (*driverRename)(virDomainObjPtr, ...),
>>                            int (*rollBack)(virDomainObjPtr, ...));
>>
>> This function will encapsulate the renaming and at the correct spot it
>> will call driverRename() so that driver can adjust its internal state.
>> If the driver is successful, just remove the old entry. If it is not,
>> call rollBack() callback which will cancel partially performed operation
>> and restore original state of the driver.
>> Moreover, in the virDomainObjListRename() we can ensure the locking
>> order and we don't need to introduce new BeginJob() API with no timeout.
>
>But very much this approach I like :-)
>

That's what I meant with:
  "Or the other way would be moving parts of the rename code into this file
   to make it available for other drivers as well."

but you explained it in more detail and understandably.  ACK for that
idea as well.

>Jirka
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160126/335949fa/attachment-0001.sig>