----- Original Message -----
> From: "Peter Krempa" <pkre...@redhat.com>
> To: "Francesco Romani" <from...@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 <from...@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.
--
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
