On 09/09/14 18:04, Francesco Romani wrote:
----- Original Message -----
From: "Peter Krempa" <pkrempa@redhat.com> To: "Francesco Romani" <fromani@redhat.com>, libvir-list@redhat.com Sent: Tuesday, September 9, 2014 2:14:23 PM Subject: Re: [libvirt] [PATCH 1/8] qemu: bulk stats: extend internal collection API
On 09/08/14 15:05, Francesco Romani wrote:
Future patches which will implement more bulk stats groups for QEMU will need to access the connection object.
To accomodate that, a few changes are needed:
* enrich internal prototype to pass connection object. * add per-group flag to mark if one collector needs monitor access or not. * if at least one collector of the requested stats needs monitor access, thus we must start a query job for each domain. The specific collectors will run nested monitor jobs inside that.
Signed-off-by: Francesco Romani <fromani@redhat.com> --- src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d724eeb..2950a4b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
@@ -17338,7 +17339,8 @@ qemuDomainGetStatsState(virDomainObjPtr dom,
typedef int -(*qemuDomainGetStatsFunc)(virDomainObjPtr dom, +(*qemuDomainGetStatsFunc)(virConnectPtr conn,
Looking through the rest of the series. Rather than the complete connection object you need just the virQEMUDriverPtr for entering the monitor, but I can live with this.
Since I need to resubmit to address your comments, I'll fix this to pass just virQEMUDriverPtr.
- if (qemuDomainGetStats(conn, dom, stats, &tmp, flags) < 0) + if (needmon && qemuDomainObjBeginJob(driver, dom, QEMU_JOB_QUERY) < 0) goto cleanup;
- if (tmp) - tmpstats[nstats++] = tmp; + if ((needmon && virDomainObjIsActive(dom)) || !needmon) {
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) { if (qemuDomainGetStats(conn, dom, stats, &tmp, privflags) < 0) /* 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.
Peter