[libvirt] [PATCH v2 06/14] conf: virDomainObjListRemoveLocked function

[Jim Fehlig]

On 06/20/2013 10:24 AM, Marek Marczykowski-Górecki wrote:
> On 20.06.2013 17:34, Jim Fehlig wrote:
>> Marek Marczykowski-Górecki wrote:
>>> While iterating with virDomainObjListForEach it is safe to remove
>>> current element. But while iterating, 'doms' lock is already taken, so
>>> can't use standard virDomainObjListRemove. So introduce
>>> virDomainObjListRemoveLocked for this purpose.
>>>
>>> Changes in v2:
>>>   - fix indentation
>>>
>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek at invisiblethingslab.com>
>>> ---
>>>   src/conf/domain_conf.c   | 17 +++++++++++++++++
>>>   src/conf/domain_conf.h   |  2 ++
>>>   src/libvirt_private.syms |  1 +
>>>   3 files changed, 20 insertions(+)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 5350c56..a4010da 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -2336,6 +2336,23 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
>>>       virObjectUnlock(doms);
>>>   }
>>>   
>>> +/* The caller must hold lock on 'doms' in addition to 'virDomainObjListRemove'
>>> + * requirements
>>> + *
>>> + * Can be used to remove current element while iterating with
>>> + * virDomainObjListForEach
>>> + */
>>> +void virDomainObjListRemoveLocked(virDomainObjListPtr doms,
>>> +                                  virDomainObjPtr dom)
>>> +{
>>> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
>>> +
>>> +    virUUIDFormat(dom->def->uuid, uuidstr);
>>> +    virObjectUnlock(dom);
>>> +
>>> +    virHashRemoveEntry(doms->objs, uuidstr);
>>>    
>> Peter fixed a race in virDomainObjListRemove with commit b7c98329, and
>> although this function expects the domain obj list to be locked on
>> entry, it still seems some aspect of the race might exist here.  Is it
>> possible for another thread to lock the dom and execute some code on it
>> after the freeing function has been triggered?
> The next patch uses it only in libxlReconnectDomain (at driver
> initialization), with driver lock hold whole the time from the vm object
> creation. So in this particular case no other thread can access this vm object.
>
> In general case I think it is still safe. The other thread need to have lock
> on 'doms' list to get access to 'dom' object. Because the caller also must
> have a lock on the list before a) getting lock on a 'dom' object, b) calling
> virDomainObjListRemoveLocked, it isn't possible for the other thread to access
> 'dom' object before caller release lock on the list.

Ah, I think I got it...

>
> Hmm, this description isn't clear... So let me try to enumerate calls:
> Thread A (which removes the entry from the list):
> 1. virObjectLock(doms)
> 2. virObjectLock(dom)
> 3. virDomainObjListRemoveLocked(doms, dom)
> 4.   virObjectUnlock(dom)
> 5.   virHashRemoveEntry
> 6. virObjectUnlock(doms)
>
> Above sequence can happen during virDomainObjListForEach.
>
> If thread B tries to get lock on dom object, it should call:
> 1. virObjectLock(doms) (e.g. as part of *Lookup function)
> 1a. (lookup here)
> 2. virObjectLock(dom)
>
> At no point thread B can get lock on 'doms' while thread A is in the middle of
> above code, so it can't reach dom object to get lock on it and use it. If
> thread B have lock on 'doms' or 'dom' before thread A starts above code,
> thread A will block until B finish.

and now I do, given the clarification.  Thanks.

This has been on the list for a while with no additional comments, so ACK and 
pushed.

Regards,
Jim