Re: [libvirt] [PATCH 01/11] qemu: extract helper to get the current balloon
----- Original Message -----
From: "Francesco Romani" <fromani(a)redhat.com>
To: libvir-list(a)redhat.com
Sent: Wednesday, September 3, 2014 8:41:13 AM
Subject: Re: [libvirt] [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