Re: [libvirt] [PATCH 2/2] getstats: add block.n.source stat

On 11/25/2014 02:10 AM, Peter Krempa wrote: > 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. >> >> + * "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. Sure, I can do that. Maybe I should then name it block.<num>.path or block.<num>.file, to make it obvious that it is only a file name? And I suppose we could always add other fields later if it turns out to be useful on network devices, but I won't worry about it for now.

>> * >> * 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. Should I split it into a separate patch, then?

>> - 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) Indeed, that fits with your request to limit to files.