Hi,

just a few notes on the code quality.

On Wed, Sep 7, 2022 at 12:34 PM Amneesh Singh <natto@weirdnatto.in> wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276

This patch adds a simple API for "query-stats-schemas" QMP command

Signed-off-by: Amneesh Singh <natto@weirdnatto.in>
---
 src/qemu/qemu_monitor.c      | 29 +++++++++++
 src/qemu/qemu_monitor.h      | 35 ++++++++++++++
 src/qemu/qemu_monitor_json.c | 93 ++++++++++++++++++++++++++++++++++++
 src/qemu/qemu_monitor_json.h |  4 ++
 4 files changed, 161 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index c2808c75a3..9581e90796 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4355,6 +4355,35 @@ qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type
 }


+VIR_ENUM_IMPL(qemuMonitorQueryStatsUnit,
+              QEMU_MONITOR_QUERY_STATS_UNIT_LAST,
+              "bytes",
+              "seconds",
+              "cycles",
+              "boolean",
+);
+
+
+VIR_ENUM_IMPL(qemuMonitorQueryStatsType,
+              QEMU_MONITOR_QUERY_STATS_TYPE_LAST,
+              "cumulative",
+              "instant",
+              "peak",
+              "linear-histogram",
+              "log2-histogram",
+);
+
+
+GHashTable *
+qemuMonitorQueryStatsSchema(qemuMonitor *mon,
+                            qemuMonitorQueryStatsProviderType provider_type)
+{
+    QEMU_CHECK_MONITOR_NULL(mon);
+
+    return qemuMonitorJSONQueryStatsSchema(mon, provider_type);
+}
+
+
 virJSONValue *
 qemuMonitorQueryStats(qemuMonitor *mon,
                       qemuMonitorQueryStatsTargetType target,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 63269e15bc..4c817dea20 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1521,6 +1521,41 @@ qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuMonitorQueryStatsProvider,
                               qemuMonitorQueryStatsProviderFree);

+typedef enum {
+    QEMU_MONITOR_QUERY_STATS_UNIT_BYTES,
+    QEMU_MONITOR_QUERY_STATS_UNIT_SECONDS,
+    QEMU_MONITOR_QUERY_STATS_UNIT_CYCLES,
+    QEMU_MONITOR_QUERY_STATS_UNIT_BOOLEAN,
+    QEMU_MONITOR_QUERY_STATS_UNIT_LAST,
+} qemuMonitorQueryStatsUnitType;
+
+VIR_ENUM_DECL(qemuMonitorQueryStatsUnit);
+
+typedef enum {
+    QEMU_MONITOR_QUERY_STATS_TYPE_CUMULATIVE,
+    QEMU_MONITOR_QUERY_STATS_TYPE_INSTANT,
+    QEMU_MONITOR_QUERY_STATS_TYPE_PEAK,
+    QEMU_MONITOR_QUERY_STATS_TYPE_LINEAR_HISTOGRAM,
+    QEMU_MONITOR_QUERY_STATS_TYPE_LOG2_HISTOGRAM,
+    QEMU_MONITOR_QUERY_STATS_TYPE_LAST,
+} qemuMonitorQueryStatsTypeType;
+
+VIR_ENUM_DECL(qemuMonitorQueryStatsType);
+
+typedef struct _qemuMonitorQueryStatsSchemaData qemuMonitorQueryStatsSchemaData;
+struct _qemuMonitorQueryStatsSchemaData {
+    qemuMonitorQueryStatsTargetType target;
+    qemuMonitorQueryStatsUnitType unit;
+    qemuMonitorQueryStatsTypeType type;
+    unsigned int bucket_size;
+    int base;
+    int exponent;
+};
+
+GHashTable *
+qemuMonitorQueryStatsSchema(qemuMonitor *mon,
+                            qemuMonitorQueryStatsProviderType provider_type);
+
 virJSONValue *
 qemuMonitorQueryStats(qemuMonitor *mon,
                       qemuMonitorQueryStatsTargetType target,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 70fba50e6c..f822a8908c 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -8575,6 +8575,99 @@ qemuMonitorJSONMigrateRecover(qemuMonitor *mon,
     return qemuMonitorJSONCheckError(cmd, reply);
 }

+static GHashTable *
+qemuMonitorJSONExtractQueryStatsSchema(virJSONValue *json)
+{
+    g_autoptr(GHashTable) schema = virHashNew(g_free);
+    size_t i;
+
+    for (i = 0; i < virJSONValueArraySize(json); i++) {
+        virJSONValue *obj, *stats;
+        const char *target_str;
+        int target;
+        size_t j;
+
+        obj = virJSONValueArrayGet(json, i);
+
+        if (!virJSONValueIsObject(obj))
+            continue;
+
+        stats = virJSONValueObjectGetArray(obj, "stats");
+
+        if (!virJSONValueIsArray(stats))
+            continue;
+
+        target_str = virJSONValueObjectGetString(obj, "target");
+        target = qemuMonitorQueryStatsTargetTypeFromString(target_str);
+
+        for (j = 0; j < virJSONValueArraySize(stats); j++) {
+            virJSONValue *stat = virJSONValueArrayGet(stats, j);
+            const char *name, *type_str, *unit_str;
+            qemuMonitorQueryStatsSchemaData *data;
+            int type, unit;

We try to declare each variable on a separate line, even when there is more of the same type.

In this case, I would also declare (and initialize) the variable 'stat' as the last, as its value is
checked right after its initialization.
 
+
+            if (!virJSONValueIsObject(stat))
+                continue;
+
+            name = virJSONValueObjectGetString(stat, "name");
+

I prefer checks that look like:

    if (!(name = virJSONValueObjectGetString(stat, "name")))
        continue;

But that's just my personal preference and you can ignore this note if you do not like it.

+            if (!name)
+                continue;
+
+            type_str = virJSONValueObjectGetString(stat, "type");
+            unit_str = virJSONValueObjectGetString(stat, "unit");
+            type = qemuMonitorQueryStatsTypeTypeFromString(type_str);
+            unit = qemuMonitorQueryStatsUnitTypeFromString(unit_str);
+
+            data = g_new0(qemuMonitorQueryStatsSchemaData, 1);
+            data->target = (target == -1) ? QEMU_MONITOR_QUERY_STATS_TARGET_LAST : target;
+            data->type = (type == -1) ? QEMU_MONITOR_QUERY_STATS_TYPE_LAST : type;
+            data->unit = (unit == -1) ? QEMU_MONITOR_QUERY_STATS_UNIT_LAST : unit;
+
+            if (virJSONValueObjectGetNumberInt(stat, "base", &data->base) < 0 ||
+                virJSONValueObjectGetNumberInt(stat, "exponent", &data->exponent) < 0)
+                data->base = 0, data->exponent = 0;

Definitely split this line into two separate lines please. Operator comma makes the code very unclean
and should not be used aside from a few exceptions in 'for' and 'while' loops.
 
+
+            /* a base of zero means that there is simply no scale, data->exponent is
+               set to 0 just for safety measures */
+
+            if (data->type == QEMU_MONITOR_QUERY_STATS_TYPE_LINEAR_HISTOGRAM &&
+                virJSONValueObjectGetNumberUint(stat, "bucket-size", &data->bucket_size) < 0)
+                data->bucket_size = 0;
+
+            virHashAddEntry(schema, name, data);
+        }
+    }
+
+    return g_steal_pointer(&schema);
+}
+
+GHashTable *
+qemuMonitorJSONQueryStatsSchema(qemuMonitor *mon,
+                                qemuMonitorQueryStatsProviderType provider_type)
+{
+    g_autoptr(virJSONValue) cmd = NULL;
+    g_autoptr(virJSONValue) reply = NULL;
+    virJSONValue *ret;
+
+    const char *type_str = qemuMonitorQueryStatsProviderTypeToString(provider_type);
+
+    if (!(cmd = qemuMonitorJSONMakeCommand("query-stats-schemas",
+                                           "S:provider", type_str,
+                                           NULL)))
+        return NULL;
+
+    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
+        return NULL;
+
+    if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0)
+        return NULL;
+
+    ret = virJSONValueObjectGetArray(reply, "return");
+
+    return qemuMonitorJSONExtractQueryStatsSchema(ret);
+}
+

 /**
  * qemuMonitorJSONQueryStats:
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index a53e6423df..c910e46504 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -813,6 +813,10 @@ int
 qemuMonitorJSONMigrateRecover(qemuMonitor *mon,
                               const char *uri);

+GHashTable *
+qemuMonitorJSONQueryStatsSchema(qemuMonitor *mon,
+                                qemuMonitorQueryStatsProviderType provider_type);
+
 virJSONValue *
 qemuMonitorJSONQueryStats(qemuMonitor *mon,
                           qemuMonitorQueryStatsTargetType target,
--
2.37.1


Otherwise nice:)

Kristina