[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.

>>   *
>>   * 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.

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]