On 03/09/2017 01:06 AM, Qiaowei Ren wrote:

Currently when virDomainListGetStats is called, the stats for those disabled perf events won't be showed in result. This will produce some problems for applications based on libvirt sometime, e.g. the OpenStack bug https://bugs.launchpad.net/ceilometer/+bug/1670948

This patch just show '0' in result for disabled events for virDomainListGetStats API. I guess this should be also rational to show all stats even though the events are not enabled.

Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com> --- src/qemu/qemu_driver.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)

Without involving OpenStack (e.g., use virsh output) what is the difference a user will see? I believe what you're indicating is that now whatever the server can support will be returned with a value of 0 (zero) or whatever the current value that's queried whether or not the collection of the event is enabled or not. Here's my concern - wouldn't this go against the spirit of being able to not see certain events by just disabling them? How would a client distinguish between a "real" return value of 0 vs. a because we we're not collecting this data so we return a value of 0? How do applications that have coded to that second model now view getting a 0 value back? The client should be able to handle not getting a specific value, shouldn't it? I think there is history of only returning valid values that the target supports and having the client make decisions from there. I believe it's possible to get a list of what perf has enabled. If you get that list and it doesn't contain what you want, but that's needed for something else, then the client can decide what to do rather than the server (e.g. libvirt). John

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2032fac..237bf57 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -19355,8 +19355,10 @@ qemuDomainGetStatsPerfOneEvent(virPerfPtr perf, char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; uint64_t value = 0;

- if (virPerfReadEvent(perf, type, &value) < 0) - return -1; + if (virPerfEventIsEnabled(perf, type)) { + if (virPerfReadEvent(perf, type, &value) < 0) + return -1; + }

snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "perf.%s", virPerfEventTypeToString(type)); @@ -19383,9 +19385,6 @@ qemuDomainGetStatsPerf(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, int ret = -1;

for (i = 0; i < VIR_PERF_EVENT_LAST; i++) { - if (!virPerfEventIsEnabled(priv->perf, i)) - continue; - if (qemuDomainGetStatsPerfOneEvent(priv->perf, i, record, maxparams) < 0) goto cleanup;