[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




----- 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.


-- 
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]