----- Original Message ----- > From: "Francesco Romani" <fromani@xxxxxxxxxx> > To: libvir-list@xxxxxxxxxx > Sent: Wednesday, September 3, 2014 8:41:13 AM > Subject: Re: [PATCH 01/11] qemu: extract helper to get the current balloon [...] > > > + > > > + cleanup: > > > + if (vm) > > > + virObjectUnlock(vm); > > > + return ret; > > > +} > > > > [3] Ouch. This function is unlocking vm, even though it did not obtain > > the lock. Which it kind of has to do because of the way that > > qemuDomainObjEndJob may end up invalidating vm. While transfer > > semantics are workable, they require good comments at the start of the > > function, and making sure that the caller doesn't duplicate the efforts, > > nor forget anything else. > > Will add comment documenting this. Is this sufficient or there is something > better I could do? After some thought and an enlightening chat, turns out I can do better, by changing a bit more qemuDomainGetStats: * let qemuDomainGetStats handle the job * let the helper run the monitor jobs inside the job provided by the caller (this requirement will be of course documented) This way, we will have one will * have one job with multiple monitor jobs inside and * avoid alltogether the awkward transfer semantings And this should ultimately lead to both clearer and faster code. So, I'll explore this direction first. Moreover, after of course addressing all other comments, I'm going to squash the patch which extracts the helper with the one which add the bulk stats group which will make use of it, in order to make obvious how the helpers are going to be used. V3 with all the above will be posted ASAP. Cheers, -- Francesco Romani RedHat Engineering Virtualization R & D Phone: 8261328 IRC: fromani -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list