Re: [libvirt] [PATCH 1/1] perf: list all supported events when get domain stats
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(a)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;