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

From: "Eric Blake" <eblake(a)redhat.com&gt; To: "Francesco Romani" <fromani(a)redhat.com&gt;, libvir-list(a)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(a)redhat.com&gt; > --- > 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?

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

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

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

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

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

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

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

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

> - /* 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) {'

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

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

> > - if (!found) { > + if (dev_name != NULL && !found) { if (dev_name && !found) {