Re: [PATCH 1/3] qemu_monitor: add qemuMonitorQueryStats

On Thu, Jul 14, 2022 at 7:54 AM Amneesh Singh <natto(a)weirdnatto.in&gt; wrote: > Related: https://gitlab.com/libvirt/libvirt/-/issues/276 > > This patch adds an API for the "query-stats" QMP command. > > The query returns a JSON containing the statistics based on the target, > which can either be vCPU or VM, and the providers. The API deserializes > the query result into an array of GHashMaps, which can later be used to > extract all the query statistics. GHashMaps are used to avoid traversing > the entire array to find the statistics you are looking for. This would > be a singleton array if the target is a VM since the returned JSON is > also a singleton array in that case. > > Signed-off-by: Amneesh Singh <natto(a)weirdnatto.in&gt; > --- > src/qemu/qemu_monitor.c | 70 +++++++++++++++++++ > src/qemu/qemu_monitor.h | 45 ++++++++++++ > src/qemu/qemu_monitor_json.c | 130 +++++++++++++++++++++++++++++++++++ > src/qemu/qemu_monitor_json.h | 6 ++ > 4 files changed, 251 insertions(+) > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index fda5d2f3..a07e6017 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -4541,3 +4541,73 @@ qemuMonitorMigrateRecover(qemuMonitor *mon, > > return qemuMonitorJSONMigrateRecover(mon, uri); > } > + > +VIR_ENUM_IMPL(qemuMonitorQueryStatsTarget, > + QEMU_MONITOR_QUERY_STATS_TARGET_LAST, > + "vm", > + "vcpu", > +); > + > +VIR_ENUM_IMPL(qemuMonitorQueryStatsName, > + QEMU_MONITOR_QUERY_STATS_NAME_LAST, > + "halt_poll_success_ns", > + "halt_poll_fail_ns" > +); > + > +VIR_ENUM_IMPL(qemuMonitorQueryStatsProvider, > + QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST, > + "kvm", > +); > + > +void > +qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider) > +{ > + virBitmapFree(provider->names); > + g_free(provider); > +} > + > +qemuMonitorQueryStatsProvider * > +qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType > provider_type, > + ...) > +{ > + g_autoptr(qemuMonitorQueryStatsProvider) provider = NULL; > + qemuMonitorQueryStatsNameType stat; > + va_list name_list; > + size_t sentinel = QEMU_MONITOR_QUERY_STATS_NAME_LAST; > + > + provider = g_new0(qemuMonitorQueryStatsProvider, 1); > + provider->provider = provider_type; > + provider->names = NULL; > + > + va_start(name_list, provider_type); > + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); > + > + if (stat != sentinel) { > It would be better to compare 'stat' with QEMU_MONITOR_QUERY_STATS_NAME_LAST directly without the additional variable 'sentinel'. > + provider->names = > virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); > + > + while (stat != sentinel) { > Same here. This while cycle would be better outside the if case, such as: if (stat != sentinel) provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); while (stat != sentinel) { .... } Because the 'while' cycle has the same condition as the 'if' case. > + if (virBitmapSetBit(provider->names, stat) < 0) > + return NULL; > + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); > + } > + } > + va_end(name_list); > + > + return g_steal_pointer(&provider); > +}