On 11/25/14 06:21, Eric Blake wrote:
> I noticed that for an offline domain, 'virsh domstats --block $dom'
> was producing just the domain name, with no stats. But the older
> 'virsh domblkinfo' works just fine on offline domains. Furthermore,
> I'm about to make block stats optionally more complex to cover
> backing chains, where block.count will no longer equal the number
> of <disks> for a domain. For these reasons, it is nicer if the
> statistics output always includes minimal information on every
> resource being described, with enough to correlate back to host
> resources, and even when some statistics are available only on a
> running domain.
>
> With this patch, I now see the following for an offline domain
> with one disk and an empty cdrom drive:
> $ virsh domstats --block foo
> Domain: 'foo'
> block.count=2
> block.0.name=hda
> block.0.source=/dev/sda4
> block.1.name=hdc
>
> * src/libvirt-domain.c (virConnectGetAllDomainStats)
> (virDomainListGetStats): Document new field; clarify that not all
> fields must be present for a group to be supported.
> * tools/virsh.pod (domstats): Document new field.
> * src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Return the new
> stat always, even for offline domains.
> (QEMU_ADD_NAME_PARAM): Add parameter.
> (qemuDomainGetStatsInterface): Update caller.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/libvirt-domain.c | 16 +++++++++++++---
> src/qemu/qemu_driver.c | 36 ++++++++++++++++++++----------------
> tools/virsh.pod | 1 +
> 3 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 2b0defc..8cd21ae 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -10907,6 +10907,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
> * "block.<num>.name" - name of the block device <num> as string.
> * matches the target name (vda/sda/hda) of the
> * block device.
> + * "block.<num>.source" - string describing the source of block device <num>,
> + * such as the host path of a file or device.
Fair enough as long as we document that we only return it for
non-network sources. We had quite a few problems with network sources so
I'd rather not expose it for those due to possible ambiguity.
> * "block.<num>.rd.reqs" - number of read requests as unsigned long long.
> * "block.<num>.rd.bytes" - number of read bytes as unsigned long long.
> * "block.<num>.rd.times" - total time (ns) spent on reads as
> @@ -10937,7 +10939,11 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
> *
> * Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes
> * the function return error in case some of the stat types in @stats were
> - * not recognized by the daemon.
> + * not recognized by the daemon. However, even with this flag, a hypervisor
> + * may omit individual fields within a group if the information is not
> + * available; as an extreme example, a supported group may produce zero
> + * fields for offline domains if the statistics are meaningful only for a
> + * running domain.
> *
> * Similarly to virConnectListAllDomains, @flags can contain various flags to
> * filter the list of domains to provide stats for.
> @@ -11017,9 +11023,13 @@ virConnectGetAllDomainStats(virConnectPtr conn,
> *
> * Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes
> * the function return error in case some of the stat types in @stats were
> - * not recognized by the daemon.
> + * not recognized by the daemon. However, even with this flag, a hypervisor
> + * may omit individual fields within a group if the information is not
> + * available; as an extreme example, a supported group may produce zero
> + * fields for offline domains if the statistics are meaningful only for a
> + * running domain.
> *
The code changes are not entirely related to this doc change.
> - * Note that any of the domain list filtering flags in @flags will be rejected
> + * Note that any of the domain list filtering flags in @flags may be rejected
> * by this function.
> *
> * Returns the count of returned statistics structures on success, -1 on error.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 07da3e3..9295a05 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18408,11 +18408,11 @@ do { \
> return -1; \
> } while (0)
>
> -#define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \
> +#define QEMU_ADD_NAME_PARAM(record, maxparams, type, subtype, num, name) \
> do { \
> char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> - "%s.%zu.name", type, num); \
> + "%s.%zu.%s", type, num, subtype); \
> if (virTypedParamsAddString(&(record)->params, \
> &(record)->nparams, \
> maxparams, \
> @@ -18457,7 +18457,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> memset(&tmp, 0, sizeof(tmp));
>
> QEMU_ADD_NAME_PARAM(record, maxparams,
> - "net", i, dom->def->nets[i]->ifname);
> + "net", "name", i, dom->def->nets[i]->ifname);
>
> if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) {
> virResetLastError();
> @@ -18526,19 +18526,20 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
> int rc;
> virHashTablePtr stats = NULL;
> qemuDomainObjPrivatePtr priv = dom->privateData;
> + bool abbreviated = false;
>
> - if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
> - return 0; /* it's ok, just go ahead silently */
> + if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
> + abbreviated = true; /* it's ok, just go ahead silently */
> + } else {
> + qemuDomainObjEnterMonitor(driver, dom);
> + rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats);
> + ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats));
> + qemuDomainObjExitMonitor(driver, dom);
>
> - qemuDomainObjEnterMonitor(driver, dom);
> - rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats);
> - ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats));
> - qemuDomainObjExitMonitor(driver, dom);
> -
> - if (rc < 0) {
> - virResetLastError();
> - ret = 0; /* still ok, again go ahead silently */
> - goto cleanup;
> + if (rc < 0) {
> + virResetLastError();
> + abbreviated = true; /* still ok, again go ahead silently */
> + }
> }
>
> QEMU_ADD_COUNT_PARAM(record, maxparams, "block", dom->def->ndisks);
> @@ -18547,9 +18548,12 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
> qemuBlockStats *entry;
> virDomainDiskDefPtr disk = dom->def->disks[i];
>
> - QEMU_ADD_NAME_PARAM(record, maxparams, "block", i, disk->dst);
> + QEMU_ADD_NAME_PARAM(record, maxparams, "block", "name", i, disk->dst);
> + if (disk->src && disk->src->path)
&& virStorageSourceIsLocalStorage(disk->src)
> + QEMU_ADD_NAME_PARAM(record, maxparams, "block", "source",
> + i, disk->src->path);
>
> - if (!disk->info.alias ||
> + if (abbreviated || !disk->info.alias ||
> !(entry = virHashLookup(stats, disk->info.alias)))
> continue;
>
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 7cde3fd..e038c7e 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -883,6 +883,7 @@ I<--interface> returns:
> I<--block> returns:
> "block.count" - number of block devices on this domain,
> "block.<num>.name" - name of the target of the block device <num>,
> +"block.<num>.source" - name of the source of block device <num>,
> "block.<num>.rd.reqs" - number of read requests,
> "block.<num>.rd.bytes" - number of read bytes,
> "block.<num>.rd.times" - total time (ns) spent on reads,
>
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list