----- Original Message -----
> From: "Eric Blake" <eblake@xxxxxxxxxx>
> To: "Francesco Romani" <fromani@xxxxxxxxxx>, libvir-list@xxxxxxxxxx
> Sent: Wednesday, September 3, 2014 12:14:58 AM
> Subject: Re: [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@xxxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list