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@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) {
+        provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST);
+
+        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);
+}
+
+GPtrArray *
+qemuMonitorQueryStats(qemuMonitor *mon,
+                      qemuMonitorQueryStatsTargetType target,
+                      char **vcpus,
+                      GPtrArray *providers)
+{
+    VIR_DEBUG("target=%u vcpus=%p providers=%p", target, vcpus, providers);
+
+    QEMU_CHECK_MONITOR_NULL(mon);
+
+    if (target != QEMU_MONITOR_QUERY_STATS_TARGET_VCPU && vcpus)
+        return NULL;
+
+    return qemuMonitorJSONQueryStats(mon, target, vcpus, providers);
+}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 95267ec6..344636e1 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1554,3 +1554,48 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon,
 int
 qemuMonitorMigrateRecover(qemuMonitor *mon,
                           const char *uri);
+
+typedef enum {
+    QEMU_MONITOR_QUERY_STATS_TARGET_VM,
+    QEMU_MONITOR_QUERY_STATS_TARGET_VCPU,
+    QEMU_MONITOR_QUERY_STATS_TARGET_LAST,
+} qemuMonitorQueryStatsTargetType;
+
+VIR_ENUM_DECL(qemuMonitorQueryStatsTarget);
+
+typedef enum {
+    QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS,
+    QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS,
+    QEMU_MONITOR_QUERY_STATS_NAME_LAST,
+} qemuMonitorQueryStatsNameType;
+
+VIR_ENUM_DECL(qemuMonitorQueryStatsName);
+
+typedef enum {
+    QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM,
+    QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST,
+} qemuMonitorQueryStatsProviderType;
+
+VIR_ENUM_DECL(qemuMonitorQueryStatsProvider);
+
+typedef struct _qemuMonitorQueryStatsProvider qemuMonitorQueryStatsProvider;
+struct _qemuMonitorQueryStatsProvider {
+    qemuMonitorQueryStatsProviderType provider;
+    virBitmap *names;
+};
+
+void
+qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider);
+
+qemuMonitorQueryStatsProvider *
+qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type,
+                                 ...);
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuMonitorQueryStatsProvider,
+                              qemuMonitorQueryStatsProviderFree);
+
+GPtrArray *
+qemuMonitorQueryStats(qemuMonitor *mon,
+                      qemuMonitorQueryStatsTargetType target,
+                      char **vcpus,
+                      GPtrArray *providers);
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"));
+        }
+
+        g_ptr_array_add(queried_stats, hash_table);
+    }
+
+    return g_steal_pointer(&queried_stats);
+}
+
+
+/**
+ * qemuMonitorJSONQueryStats:
+ * @mon: monitor object
+ * @target: the target type for the query
+ * @vcpus: a list of vCPU QOM paths for filtering the statistics
+ * @providers: an array of providers to filter statistics
+ *
+ * @vcpus is a NULL terminated array of strings. @providers is a GPtrArray
+ * for qemuMonitorQueryStatsProvider.
+ * @vcpus and @providers are optional and can be NULL.
+ *
+ * Queries for the @target based statistics.
+ * Returns a GPtrArray of GHashTables containing the statistics on success or
+ * NULL on failure.
+ */
+
+GPtrArray *
+qemuMonitorJSONQueryStats(qemuMonitor *mon,
+                          qemuMonitorQueryStatsTargetType target,
+                          char **vcpus,
+                          GPtrArray *providers)
+{
+    g_autoptr(virJSONValue) cmd = NULL;
+    g_autoptr(virJSONValue) reply = NULL;
+    g_autoptr(virJSONValue) vcpu_list = NULL;
+    g_autoptr(virJSONValue) provider_list = NULL;
+    virJSONValue *rv = NULL;
+
+    size_t i;
+
+    if (providers) {
+        provider_list = virJSONValueNewArray();
+
+        for (i = 0; i < providers->len; i++) {
+            g_autoptr(virJSONValue) provider_obj = virJSONValueNewObject();
+            qemuMonitorQueryStatsProvider *provider = providers->pdata[i];
+            const char *type_str = qemuMonitorQueryStatsProviderTypeToString(provider->provider);
+            virBitmap *names = provider->names;
+            int rc;
+
+            rc = virJSONValueObjectAppendString(provider_obj, "provider", type_str);
+
+            if (rc < 0)
+                return NULL;
+
+            if (names) {
+                g_autoptr(virJSONValue) provider_names = virJSONValueNewArray();
+                int curBit = virBitmapNextSetBit(names, -1);
+
+                while (curBit >= 0) {
+                    const char *name = qemuMonitorQueryStatsNameTypeToString(curBit);
+
+                    if (virJSONValueArrayAppendString(provider_names, name) < 0)
+                        return NULL;
+
+                    curBit = virBitmapNextSetBit(names, curBit);
+                }
+
+                rc = virJSONValueObjectAppend(provider_obj, "names", &provider_names);
+
+                if (rc < 0)
+                    return NULL;
+            }
+
+            if (virJSONValueArrayAppend(provider_list, &provider_obj) < 0)
+                return NULL;
+        }
+    }
+
+    if (vcpus) {
+        vcpu_list = virJSONValueNewArray();
+
+        for (i = 0; vcpus[i]; i++)
+            if (virJSONValueArrayAppendString(vcpu_list, vcpus[i]) < 0)
+                return NULL;
+    }
+
+    cmd = qemuMonitorJSONMakeCommand("query-stats",
+                                     "s:target", qemuMonitorQueryStatsTargetTypeToString(target),
+                                     "A:vcpus", &vcpu_list,
+                                     "A:providers", &provider_list,
+                                     NULL);
+
+    if (!cmd)
+        return NULL;
+
+    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
+        return NULL;
+
+    if (qemuMonitorJSONCheckError(cmd, reply) < 0)
+        return NULL;
+
+    rv = virJSONValueObjectStealArray(reply, "return");
+
+    return qemuMonitorJSONExtractQueryStats(rv);
+}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index ad3853ae..db8dec80 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -878,3 +878,9 @@ qemuMonitorJSONChangeMemoryRequestedSize(qemuMonitor *mon,
 int
 qemuMonitorJSONMigrateRecover(qemuMonitor *mon,
                               const char *uri);
+
+GPtrArray *
+qemuMonitorJSONQueryStats(qemuMonitor *mon,
+                          qemuMonitorQueryStatsTargetType target,
+                          char **vcpus,
+                          GPtrArray *providers);
-- 
2.36.1

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@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_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"));
>+        }
>+
>+        g_ptr_array_add(queried_stats, hash_table);
>+    }
>+
>+    return g_steal_pointer(&queried_stats);
>+}
>+
>+
>+/**
>+ * qemuMonitorJSONQueryStats:
>+ * @mon: monitor object
>+ * @target: the target type for the query
>+ * @vcpus: a list of vCPU QOM paths for filtering the statistics
>+ * @providers: an array of providers to filter statistics
>+ *
>+ * @vcpus is a NULL terminated array of strings. @providers is a GPtrArray
>+ * for qemuMonitorQueryStatsProvider.
>+ * @vcpus and @providers are optional and can be NULL.
>+ *
>+ * Queries for the @target based statistics.
>+ * Returns a GPtrArray of GHashTables containing the statistics on success or
>+ * NULL on failure.
>+ */
>+
>+GPtrArray *
>+qemuMonitorJSONQueryStats(qemuMonitor *mon,
>+                          qemuMonitorQueryStatsTargetType target,
>+                          char **vcpus,
>+                          GPtrArray *providers)
>+{
>+    g_autoptr(virJSONValue) cmd = NULL;
>+    g_autoptr(virJSONValue) reply = NULL;
>+    g_autoptr(virJSONValue) vcpu_list = NULL;
>+    g_autoptr(virJSONValue) provider_list = NULL;
>+    virJSONValue *rv = NULL;
>+
>+    size_t i;
>+
>+    if (providers) {
>+        provider_list = virJSONValueNewArray();
>+
>+        for (i = 0; i < providers->len; i++) {
>+            g_autoptr(virJSONValue) provider_obj = virJSONValueNewObject();
>+            qemuMonitorQueryStatsProvider *provider = providers->pdata[i];
>+            const char *type_str = qemuMonitorQueryStatsProviderTypeToString(provider->provider);
>+            virBitmap *names = provider->names;
>+            int rc;
>+
>+            rc = virJSONValueObjectAppendString(provider_obj, "provider", type_str);
>+
>+            if (rc < 0)
>+                return NULL;
>+
>+            if (names) {
>+                g_autoptr(virJSONValue) provider_names = virJSONValueNewArray();
>+                int curBit = virBitmapNextSetBit(names, -1);
>+
>+                while (curBit >= 0) {
>+                    const char *name = qemuMonitorQueryStatsNameTypeToString(curBit);
>+
>+                    if (virJSONValueArrayAppendString(provider_names, name) < 0)
>+                        return NULL;
>+
>+                    curBit = virBitmapNextSetBit(names, curBit);
>+                }

Also this can be shorter with:

int curBit = -1;

while ((curBit = virBitmapNextSetBit(names, curBit)) != -1) {
    ...
}

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@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) {
>+        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.

On Thu, Jul 14, 2022 at 7:54 AM Amneesh Singh <natto@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@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);
> +}
> +
> +GPtrArray *
> +qemuMonitorQueryStats(qemuMonitor *mon,
> +                      qemuMonitorQueryStatsTargetType target,
> +                      char **vcpus,
> +                      GPtrArray *providers)
> +{
> +    VIR_DEBUG("target=%u vcpus=%p providers=%p", target, vcpus,
> providers);
> +
> +    QEMU_CHECK_MONITOR_NULL(mon);
> +
> +    if (target != QEMU_MONITOR_QUERY_STATS_TARGET_VCPU && vcpus)
> +        return NULL;
> +
> +    return qemuMonitorJSONQueryStats(mon, target, vcpus, providers);
> +}
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 95267ec6..344636e1 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1554,3 +1554,48 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor
> *mon,
>  int
>  qemuMonitorMigrateRecover(qemuMonitor *mon,
>                            const char *uri);
> +
> +typedef enum {
> +    QEMU_MONITOR_QUERY_STATS_TARGET_VM,
> +    QEMU_MONITOR_QUERY_STATS_TARGET_VCPU,
> +    QEMU_MONITOR_QUERY_STATS_TARGET_LAST,
> +} qemuMonitorQueryStatsTargetType;
> +
> +VIR_ENUM_DECL(qemuMonitorQueryStatsTarget);
> +
> +typedef enum {
> +    QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS,
> +    QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS,
> +    QEMU_MONITOR_QUERY_STATS_NAME_LAST,
> +} qemuMonitorQueryStatsNameType;
> +
> +VIR_ENUM_DECL(qemuMonitorQueryStatsName);
> +
> +typedef enum {
> +    QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM,
> +    QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST,
> +} qemuMonitorQueryStatsProviderType;
> +
> +VIR_ENUM_DECL(qemuMonitorQueryStatsProvider);
> +
> +typedef struct _qemuMonitorQueryStatsProvider
> qemuMonitorQueryStatsProvider;
> +struct _qemuMonitorQueryStatsProvider {
> +    qemuMonitorQueryStatsProviderType provider;
>

It is a bit confusing for me to have a variable 'provider' in the
'provider' structure.
Later you use provider->provider which is not very readable.
I would prefer 'type' (maybe even 'providerType') instead.


> +    virBitmap *names;
> +};
> +
> +void
> +qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider
> *provider);
> +
> +qemuMonitorQueryStatsProvider *
> +qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType
> provider_type,
> +                                 ...);
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuMonitorQueryStatsProvider,
> +                              qemuMonitorQueryStatsProviderFree);
> +
> +GPtrArray *
> +qemuMonitorQueryStats(qemuMonitor *mon,
> +                      qemuMonitorQueryStatsTargetType target,
> +                      char **vcpus,
> +                      GPtrArray *providers);
> 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"));
> +        }
> +
> +        g_ptr_array_add(queried_stats, hash_table);
> +    }
> +
> +    return g_steal_pointer(&queried_stats);
> +}
> +
> +
> +/**
> + * qemuMonitorJSONQueryStats:
> + * @mon: monitor object
> + * @target: the target type for the query
> + * @vcpus: a list of vCPU QOM paths for filtering the statistics
> + * @providers: an array of providers to filter statistics
> + *
> + * @vcpus is a NULL terminated array of strings. @providers is a GPtrArray
> + * for qemuMonitorQueryStatsProvider.
> + * @vcpus and @providers are optional and can be NULL.
> + *
> + * Queries for the @target based statistics.
> + * Returns a GPtrArray of GHashTables containing the statistics on
> success or
> + * NULL on failure.
> + */
> +
> +GPtrArray *
> +qemuMonitorJSONQueryStats(qemuMonitor *mon,
> +                          qemuMonitorQueryStatsTargetType target,
> +                          char **vcpus,
> +                          GPtrArray *providers)
> +{
> +    g_autoptr(virJSONValue) cmd = NULL;
> +    g_autoptr(virJSONValue) reply = NULL;
> +    g_autoptr(virJSONValue) vcpu_list = NULL;
> +    g_autoptr(virJSONValue) provider_list = NULL;
> +    virJSONValue *rv = NULL;
> +
> +    size_t i;
> +
> +    if (providers) {
> +        provider_list = virJSONValueNewArray();
> +
> +        for (i = 0; i < providers->len; i++) {
> +            g_autoptr(virJSONValue) provider_obj =
> virJSONValueNewObject();
> +            qemuMonitorQueryStatsProvider *provider = providers->pdata[i];
> +            const char *type_str =
> qemuMonitorQueryStatsProviderTypeToString(provider->provider);
> +            virBitmap *names = provider->names;
> +            int rc;
> +
>

We usually use 'rc' (or any variable storing the result) if we need the
value also after the
check of its return code. When we only need to check the result, we usually
compare the
value directly:

if (virJSONValueObjectAppendString(provider_obj, "provider", type_str) < 0)
    return NULL;


> +            rc = virJSONValueObjectAppendString(provider_obj, "provider",
> type_str);
> +
> +            if (rc < 0)
> +                return NULL;
> +
> +            if (names) {
> +                g_autoptr(virJSONValue) provider_names =
> virJSONValueNewArray();
> +                int curBit = virBitmapNextSetBit(names, -1);
> +
> +                while (curBit >= 0) {
> +                    const char *name =
> qemuMonitorQueryStatsNameTypeToString(curBit);
> +
> +                    if (virJSONValueArrayAppendString(provider_names,
> name) < 0)
> +                        return NULL;
> +
> +                    curBit = virBitmapNextSetBit(names, curBit);
> +                }
> +
> +                rc = virJSONValueObjectAppend(provider_obj, "names",
> &provider_names);
> +
>

Same here


> +                if (rc < 0)
> +                    return NULL;
> +            }
> +
> +            if (virJSONValueArrayAppend(provider_list, &provider_obj) < 0)
> +                return NULL;
> +        }
> +    }
> +
> +    if (vcpus) {
> +        vcpu_list = virJSONValueNewArray();
> +
> +        for (i = 0; vcpus[i]; i++)
> +            if (virJSONValueArrayAppendString(vcpu_list, vcpus[i]) < 0)
> +                return NULL;
> +    }
> +
> +    cmd = qemuMonitorJSONMakeCommand("query-stats",
> +                                     "s:target",
> qemuMonitorQueryStatsTargetTypeToString(target),
> +                                     "A:vcpus", &vcpu_list,
> +                                     "A:providers", &provider_list,
> +                                     NULL);
> +
> +    if (!cmd)
> +        return NULL;
> +
> +    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> +        return NULL;
> +
> +    if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> +        return NULL;
> +
> +    rv = virJSONValueObjectStealArray(reply, "return");
> +
> +    return qemuMonitorJSONExtractQueryStats(rv);
> +}
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index ad3853ae..db8dec80 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -878,3 +878,9 @@ qemuMonitorJSONChangeMemoryRequestedSize(qemuMonitor
> *mon,
>  int
>  qemuMonitorJSONMigrateRecover(qemuMonitor *mon,
>                                const char *uri);
> +
> +GPtrArray *
> +qemuMonitorJSONQueryStats(qemuMonitor *mon,
> +                          qemuMonitorQueryStatsTargetType target,
> +                          char **vcpus,
> +                          GPtrArray *providers);
> --
> 2.36.1
>
>

Kind regards,
Kristina

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

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

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@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@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.h b/src/qemu/qemu_monitor.h
>> index 95267ec6..344636e1 100644
>> --- a/src/qemu/qemu_monitor.h
>> +++ b/src/qemu/qemu_monitor.h
>> @@ -1554,3 +1554,48 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor
>> *mon,
>>  int
>>  qemuMonitorMigrateRecover(qemuMonitor *mon,
>>                            const char *uri);
>> +
>> +typedef enum {
>> +    QEMU_MONITOR_QUERY_STATS_TARGET_VM,
>> +    QEMU_MONITOR_QUERY_STATS_TARGET_VCPU,
>> +    QEMU_MONITOR_QUERY_STATS_TARGET_LAST,
>> +} qemuMonitorQueryStatsTargetType;
>> +
>> +VIR_ENUM_DECL(qemuMonitorQueryStatsTarget);
>> +
>> +typedef enum {
>> +    QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS,
>> +    QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS,
>> +    QEMU_MONITOR_QUERY_STATS_NAME_LAST,
>> +} qemuMonitorQueryStatsNameType;
>> +
>> +VIR_ENUM_DECL(qemuMonitorQueryStatsName);
>> +
>> +typedef enum {
>> +    QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM,
>> +    QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST,
>> +} qemuMonitorQueryStatsProviderType;
>> +
>> +VIR_ENUM_DECL(qemuMonitorQueryStatsProvider);
>> +
>> +typedef struct _qemuMonitorQueryStatsProvider
>> qemuMonitorQueryStatsProvider;
>> +struct _qemuMonitorQueryStatsProvider {
>> +    qemuMonitorQueryStatsProviderType provider;
>>
>
>It is a bit confusing for me to have a variable 'provider' in the
>'provider' structure.
>Later you use provider->provider which is not very readable.
>I would prefer 'type' (maybe even 'providerType') instead.
>

I think I suggested `type` in previous review.

I'll fix this up and the other comments (mine as well) before pushing unless
there is something bigger in the last patch.