[libvirt] [PATCHv4 6/8] qemu: bulk stats: implement block group

[Francesco Romani]

----- Original Message -----
> From: "Peter Krempa" <pkrempa at redhat.com>
> To: "Francesco Romani" <fromani at redhat.com>, libvir-list at redhat.com
> Sent: Friday, September 12, 2014 3:56:06 PM
> Subject: Re: [libvirt] [PATCHv4 6/8] qemu: bulk stats: implement block group
> 
> On 09/12/14 13:48, Francesco Romani wrote:
> > This patch implements the VIR_DOMAIN_STATS_BLOCK
> > group of statistics.
> > 
> > To do so, an helper function to get the block stats
> > of all the disks of a domain is added.
> > 
> > Signed-off-by: Francesco Romani <fromani at redhat.com>
> > ---
> >  include/libvirt/libvirt.h.in |   1 +
> >  src/libvirt.c                |  20 +++++++
> >  src/qemu/qemu_driver.c       |  96 ++++++++++++++++++++++++++++++
> >  src/qemu/qemu_monitor.c      |  26 +++++++++
> >  src/qemu/qemu_monitor.h      |  20 +++++++
> >  src/qemu/qemu_monitor_json.c | 136
> >  +++++++++++++++++++++++++++++--------------
> >  src/qemu/qemu_monitor_json.h |   4 ++
> >  7 files changed, 260 insertions(+), 43 deletions(-)
> > 
> 
> 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 7e5d707..4644f4a 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -9687,6 +9687,31 @@ qemuDomainBlockStats(virDomainPtr dom,
> >      return ret;
> >  }
> >  
> > +
> > +/*
> > + * Returns at most the first `nstats' stats, then stops.
> > + * Returns the number of stats filled.
> > + */
> > +static int
> > +qemuDomainHelperGetBlockStats(virQEMUDriverPtr driver,
> > +                              virDomainObjPtr vm,
> > +                              qemuBlockStatsPtr stats,
> > +                              int nstats)
> > +{
> > +    int ret;
> > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > +
> > +    qemuDomainObjEnterMonitor(driver, vm);
> > +
> > +    ret = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL,
> > +                                          stats, nstats);
> 
> 
> Humm, is it worth doing this helper? This pretty much can be inlined as
> it has only one caller.

Right, qemuDomainHelperGetBlockStats add little to none value, so I'll drop it.
I believe qemuMonitorGetAllBlockStatsInfo should stay, however:
I don't see JSON monitor being called directly anywhere. So I'll keep it.

[...]
> > +static int
> > +qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
> > +                        virDomainObjPtr dom,
> > +                        virDomainStatsRecordPtr record,
> > +                        int *maxparams,
> > +                        unsigned int privflags)
> > +{
> > +    size_t i;
> > +    int ret = -1;
> > +    int nstats = 0;
> > +    qemuBlockStatsPtr stats = NULL;
> > +
> > +    if (!HAVE_MONITOR(privflags) || !virDomainObjIsActive(dom))
> > +        return 0; /* it's ok, just go ahead silently */
> > +
> > +    if (VIR_ALLOC_N(stats, dom->def->ndisks) < 0)
> > +        return -1;
> > +
> > +    nstats = qemuDomainHelperGetBlockStats(driver, dom, stats,
> > +                                           dom->def->ndisks);
> > +    if (nstats < 0)
> 
> Are we erroring out on block stats failure? Other statistics gatherers
> just skip it if it's not available.

Right, will fix to make it silently skip as the other groups.

Bests,

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