On 09/10/14 15:55, Francesco Romani wrote:
----- Original Message -----
From: "Peter Krempa" <pkrempa@redhat.com> To: "Francesco Romani" <fromani@redhat.com>, libvir-list@redhat.com Sent: Wednesday, September 10, 2014 10:07:33 AM Subject: Re: [libvirt] [PATCH 1/8] qemu: bulk stats: extend internal collection API
[...]
Hmm this skips offline domains entirely if one of the stats groups needs the monitor.
I think we should rather skip individual stats groups, or better stats fields that we can't provide.
Any ideas?
What about this (pseudo-C):
unsigned int privflags = 0;
if (needmon && qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) needmon = false; /* auto disable monitoring, the remainder of the function should be unchanged */ else privflags |= MONITOR_AVAILABLE;
if ((needmon && virDomainObjIsActive(dom)) || !needmon) {
Drop this condition ...
if (qemuDomainGetStats(conn, dom, stats, &tmp, privflags) < 0)
The individual stats groups will decide if they need the VM alive they can skip it individually rather than omitting the vm completely.
/* pass monitor availability down the chain. Individual workers will bail out immediately and silently if they need monitor but it is not available */ goto endjob;
if (tmp) tmpstats[nstats++] = tmp; }
No other change should be needed to this patch, and with trivial changes all the others can be fixed.
Also you can just grab the lock always and the workers will exit if the VM is not alive. Having a domain job should be fine.
As you prefer. For oVirt, we do care of all the stats group implemented, and always all of them, (actually I may have missed some bits, e.g. the pininfo as you pointed out elsewhere - going to fix), so for our needs we'll always need to enter the monitor.
But other users of this API may beg to differ, and I believe is fair to assume that other consumers of this API could just use stats which doesn't require to enter the monitor: e.g. CPU/VCPU/interface.
In case you don't have any stat group that requires the monitor it is imho fine not to get a job. If there is a group where we can enter it we can just grab the monitor always ... see above.
Hence, I was trying to be a good citizen and do not require monitor access unless it is actually needed; but if turns out it is OK to do a domain job anyway, I'll happily simplify my code :)
Thanks and bests,
Peter