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

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.

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.

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.