12:03 a.m.
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(a)weirdnatto.in> 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>
> > ---
> > 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