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?
> virQEMUDriverPtr qemu_driver = NULL;
>
>
> @@ -9672,6 +9678,41 @@ qemuDomainBlockStats(virDomainPtr dom,
> return ret;
> }
>
> +
> +/*
> + * returns at most the first `nstats' stats, then stops.
> + * Returns the number of stats filled.
> + */
> +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.
> + return ret;
> +}
> +
> static int
> qemuDomainBlockStatsFlags(virDomainPtr dom,
> const char *path,
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 5b2952a..8aadba5 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1754,6 +1754,29 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon,
> return ret;
> }
>
> +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.
> +
> + return ret;
> +}
> +
> /* Return 0 and update @nparams with the number of block stats
> * QEMU supports if success. Return -1 if failure.
> */
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 4fd6f01..1b7d00b 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -346,6 +346,24 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon,
> long long *flush_req,
> long long *flush_total_times,
> long long *errs);
> +
> +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.
> +};
> +
> +int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
> + const char *dev_name,
> + struct qemuBlockStats *stats,
> + int nstats);
> +
> int qemuMonitorGetBlockStatsParamsNumber(qemuMonitorPtr mon,
> int *nparams);
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 62e7d5d..68c5cf8 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1712,13 +1712,9 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
> long long *flush_total_times,
> long long *errs)
> {
> - int ret;
> - size_t i;
> - bool found = false;
> - virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats",
> - NULL);
> - virJSONValuePtr reply = NULL;
> - virJSONValuePtr devices;
> + struct qemuBlockStats stats;
> + int nstats = 1;
> + int ret = -1;
>
> *rd_req = *rd_bytes = -1;
> *wr_req = *wr_bytes = *errs = -1;
> @@ -1732,9 +1728,45 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
> if (flush_total_times)
> *flush_total_times = -1;
>
> + if (qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name, &stats, nstats) != nstats)
> + goto cleanup;
> +
> + *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) {
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list