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@redhat.com> --- 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