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

Re: [libvirt] [PATCHv2 03/11] qemu: add helper to get the block stats



----- Original Message -----
> From: "Eric Blake" <eblake redhat com>
> To: "Francesco Romani" <fromani redhat com>, libvir-list redhat com
> Sent: Wednesday, September 3, 2014 12:14:58 AM
> Subject: Re: [libvirt] [PATCHv2 03/11] qemu: add helper to get the block	stats
> 
> On 09/02/2014 06:31 AM, Francesco Romani wrote:
> > Add an helper function to get the block stats
> > of a disk.
> > This helper is meant to be used by the bulk stats API.
> > 
> > Signed-off-by: Francesco Romani <fromani redhat com>
> > ---
> >  src/qemu/qemu_driver.c       |  41 +++++++++++++++
> >  src/qemu/qemu_monitor.c      |  23 +++++++++
> >  src/qemu/qemu_monitor.h      |  18 +++++++
> >  src/qemu/qemu_monitor_json.c | 118
> >  +++++++++++++++++++++++++++++--------------
> >  src/qemu/qemu_monitor_json.h |   4 ++
> >  5 files changed, 165 insertions(+), 39 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 1842e60..39e2c1b 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -178,6 +178,12 @@ static int qemuDomainHelperGetVcpus(virDomainObjPtr
> > vm,
> >                                      unsigned char *cpumaps,
> >                                      int maplen);
> >  
> > +static int qemuDomainGetBlockStats(virQEMUDriverPtr driver,
> > +                                   virDomainObjPtr vm,
> > +                                   struct qemuBlockStats *stats,
> > +                                   int nstats);
> > +
> > +
> 
> Another forward declaration to be avoided.
> 
> Why do you need 'struct qemuBlockStats' in the declaration? Did you
> forget a typedef somewhere?

I saw an internal struct typedef-less (qemuAutostartData maybe?)
and this somehow got stuck on my mind.
But I believe modern libvirt style mandates typedef, is that correct?
I'll amend my code accordingly.

> > +static int
> > +qemuDomainGetBlockStats(virQEMUDriverPtr driver,
> > +                        virDomainObjPtr vm,
> > +                        struct qemuBlockStats *stats,
> > +                        int nstats)
> > +{
> > +    int ret = -1;
> > +    qemuDomainObjPrivatePtr priv;
> > +
> > +    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> > +        goto cleanup;
> > +
> > +    priv = vm->privateData;
> > +
> > +    qemuDomainObjEnterMonitor(driver, vm);
> 
> Missing a call to check if the domain is still running.  The mere act of
> calling qemuDomainObjBeginJob causes us to temporarily drop locks, and
> while the locks are down, the VM can shutdown independently and that
> makes the monitor go away; but qemuDomainObjEnterMonitor is only safe to
> call if we know the monitor still exists.  There should be plenty of
> examples to copy from.

Thanks for the explanation, will add the check.

> > +
> > +    ret = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL,
> > +                                          stats, nstats);
> > +
> > +    qemuDomainObjExitMonitor(driver, vm);
> > +
> > +    if (!qemuDomainObjEndJob(driver, vm))
> > +        vm = NULL;
> > +
> > + cleanup:
> > +    if (vm)
> > +        virObjectUnlock(vm);
> 
> Another case of required transfer semantics.
> 
> Is this patch complete? I don't see any caller of the new
> qemuDomainGetBlockStats, and gcc generally gives a compiler warning
> (which then turns into a failed build due to -Werror) if you have an
> unused static function.

It does. I (wrongly) thought it is easier/better to have little patches
so one which add code, one which makes use of it (10/11 in this series).
Will squash with the dependent patch on the next submission.

> > +int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
> 
> Style: two blank lines between functions, return type on its own line.

Oops. Will fix.
 
> > +                                    const char *dev_name,
> > +                                    struct qemuBlockStats *stats,
> > +                                    int nstats)
> > +{
> > +    int ret;
> > +    VIR_DEBUG("mon=%p dev=%s", mon, dev_name);
> > +
> > +    if (!mon) {
> > +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> > +                       _("monitor must not be NULL"));
> > +        return -1;
> > +    }
> 
> This if can be nuked if you'd just mark the mon parameter as
> ATTRIBUTE_NONNULL (all our callers use it correctly, after all).

Will do.
 
> > +
> > +    if (mon->json)
> > +        ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name,
> > +                                                  stats, nstats);
> > +    else
> > +        ret = -1; /* not supported */
> 
> Returning -1 without printing an error message is bad.

Will add.

> > +
> > +struct qemuBlockStats {
> > +    long long rd_req;
> > +    long long rd_bytes;
> > +    long long wr_req;
> > +    long long wr_bytes;
> > +    long long rd_total_times;
> > +    long long wr_total_times;
> > +    long long flush_req;
> > +    long long flush_total_times;
> > +    long long errs; /* meaningless for QEMU */
> 
> Umm, why do we need an 'errs' parameter, if it is meaningless?  I can
> see that this is sort of a superset of the public virDomainBlockStats
> struct, but that struct is generic to multiple hypervisors; and it also
> looks like the additional fields correspond to typed parameters
> available from virDomainBlockStatsFlags.  But I see no point in
> reporting an 'errs' typed param if it makes no sense for qemu.

Will purge the 'errs' parameter. It was added to closely mimic
the originating code (virDomainGetBlockInfo), but turns out
I took this too strictly and become cargo cult-ish.
 
> > +    *rd_req = stats.rd_req;
> > +    *rd_bytes = stats.rd_bytes;
> > +    *rd_total_times = stats.rd_total_times;
> > +    *wr_req = stats.wr_req;
> > +    *wr_bytes = stats.wr_bytes;
> > +    *wr_total_times = stats.wr_total_times;
> > +    *flush_req = stats.flush_req;
> > +    *flush_total_times = stats.flush_total_times;
> > +    *errs = stats.errs;
> 
> Are you guaranteed that all of these pointers are non-NULL and that you
> can blindly assign into them?  A quick grep shows that at least
> qemuDomainBlockStats passes in several NULLs.

Will add checks in this function to handle NULLs.


> 
> > +
> > +int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
> 
> Style: return type on its own line.

Will do.

> 
> > -        /* New QEMU has separate names for host & guest side of the disk
> > -         * and libvirt gives the host side a 'drive-' prefix. The passed
> > -         * in dev_name is the guest side though
> > -         */
> > -        if (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX))
> > -            thisdev += strlen(QEMU_DRIVE_HOST_PREFIX);
> > +        if (dev_name != NULL) {
> 
> Can be written as just 'if (dev_name) {'

Will do.

> 
> >          }
> > -        if (virJSONValueObjectGetNumberLong(stats, "wr_operations",
> > wr_req) < 0) {
> > +        if (virJSONValueObjectGetNumberLong(stats, "wr_operations",
> > &blockstats->wr_req) < 0) {
> 
> Some long lines, worth wrapping to keep under 80 columns.

Will fix here and everywhere else.

> >              virReportError(VIR_ERR_INTERNAL_ERROR,
> >                             _("cannot read %s statistic"),
> >                             "wr_operations");
> >              goto cleanup;
> >          }
> > -        if (wr_total_times &&
> > -            virJSONValueObjectHasKey(stats, "wr_total_time_ns") &&
> > +        if (virJSONValueObjectHasKey(stats, "wr_total_time_ns") &&
> >              (virJSONValueObjectGetNumberLong(stats, "wr_total_time_ns",
> > -                                             wr_total_times) < 0)) {
> 
> Over-parenthesized (here and several other places).

Will fix everywhere.
 
> >  
> > -    if (!found) {
> > +    if (dev_name != NULL && !found) {
> 
> if (dev_name && !found) {

Will fix.

thanks,

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