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

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(a)invisiblethingslab.com&gt; >> --- >> 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.

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.