On Fri, Jul 22, 2022 at 05:02:06PM +0200, Martin Kletzander wrote: > On Fri, Jul 22, 2022 at 02:44:30PM +0200, Kristina Hanicova wrote: > > On Thu, Jul 14, 2022 at 7:54 AM Amneesh Singh <natto@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > > > --- > > > 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); > > > +} > > Sorry for so many review e-mails from me on this one patch, but while fixing few > of the nitpicks I managed to rewrite the function quite a bit: > > qemuMonitorQueryStatsProvider *provider = g_new0(qemuMonitorQueryStatsProvider, 1); > qemuMonitorQueryStatsNameType stat; > va_list name_list; > > /* > * This can be lowered later in case of the enum getting quite large, hence > * the virBitmapSetExpand below. > */ > provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); > provider->type = provider_type; > > va_start(name_list, provider_type); > > while ((stat = va_arg(name_list, qemuMonitorQueryStatsNameType)) != > QEMU_MONITOR_QUERY_STATS_NAME_LAST) > virBitmapSetBitExpand(provider->names, stat); > > va_end(name_list); > > return provider; > > Let me know (both of you) if you are okay with me using this version. Yes, that is absolutely fine, thanks for taking the time to review it. I will keep the suggestions in mind the next time I submit the patches. > > Thanks
Attachment:
signature.asc
Description: PGP signature
