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

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 :-)

Jirka -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list