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