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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux