[PATCH 1/3] qemu_monitor: add qemuMonitorQueryStats

[Martin Kletzander]

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

Thanks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220722/c3cb0ea5/attachment-0001.sig>