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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Thu, Jul 14, 2022 at 11:22:02AM +0530, 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 <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) {
> +        provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST);
> +

It is very unlikely to happen to just get passed an empty set of names, and even
in that case it would not cause any issues to have an empty virBitmap.  I'd just
allocate it in any case to make the code nicer.

Also it would not crash in QueryStats when searching for the set bits.

> +        while (stat != sentinel) {
> +            if (virBitmapSetBit(provider->names, stat) < 0)
> +                return NULL;
> +            stat = va_arg(name_list, qemuMonitorQueryStatsNameType);
> +        }
> +    }
> +    va_end(name_list);
> +
> +    return g_steal_pointer(&provider);
> +}
> +
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 80696de7..4e228bfb 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -9031,3 +9031,133 @@ qemuMonitorJSONMigrateRecover(qemuMonitor *mon,
>
>     return qemuMonitorJSONCheckError(cmd, reply);
> }
> +
> +static GPtrArray *
> +qemuMonitorJSONExtractQueryStats(virJSONValue *arr)
> +{
> +    g_autoptr(GPtrArray) queried_stats = NULL;
> +    size_t nstats = virJSONValueArraySize(arr);
> +    size_t i;
> +
> +    /* Create a GPtrArray for GHashTables */
> +    queried_stats = g_ptr_array_new_full(nstats, (GDestroyNotify) g_hash_table_destroy);
> +
> +    for (i = 0; i < nstats; i++) {
> +        virJSONValue *obj = virJSONValueArrayGet(arr, i);
> +        virJSONValue *stats = virJSONValueObjectGetArray(obj, "stats");
> +        size_t j;
> +
> +        /* Create a GHashTable for virJSONValues */
> +        GHashTable *hash_table = virHashNew((GDestroyNotify) virJSONValueFree);
> +
> +        for (j = 0; j < virJSONValueArraySize(stats); j++) {
> +            virJSONValue *stat = virJSONValueArrayGet(stats, j);
> +
> +            g_hash_table_insert(hash_table,
> +                                g_strdup(virJSONValueObjectGetString(stat, "name")),
> +                                virJSONValueObjectGet(stat, "value"));

Similarly to the other checks we should also check these virJSONObjectGet*
functions do not return NULL, just in case.  The reasoning behind it is that we
definitely do not want a (possibly exploited) qemu to be able to crash libvirt.

Attachment: signature.asc
Description: PGP signature