Re: [libvirt] [PATCH 1/8] qemu: bulk stats: extend internal collection API

----- Original Message ----- > From: "Peter Krempa" <pkrempa(a)redhat.com&gt; > To: "Francesco Romani" <fromani(a)redhat.com&gt;, libvir-list(a)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(a)redhat.com&gt; >> --- >> 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.