<div dir="ltr"><div>Hi,</div><div><br></div><div>just a few notes on the code quality.<br></div><div><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Sep 7, 2022 at 12:34 PM Amneesh Singh <<a href="mailto:natto@weirdnatto.in">natto@weirdnatto.in</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Related: <a href="https://gitlab.com/libvirt/libvirt/-/issues/276" rel="noreferrer" target="_blank">https://gitlab.com/libvirt/libvirt/-/issues/276</a><br>
<br>
This patch adds a simple API for "query-stats-schemas" QMP command<br>
<br>
Signed-off-by: Amneesh Singh <<a href="mailto:natto@weirdnatto.in" target="_blank">natto@weirdnatto.in</a>><br>
---<br>
 src/qemu/qemu_monitor.c      | 29 +++++++++++<br>
 src/qemu/qemu_monitor.h      | 35 ++++++++++++++<br>
 src/qemu/qemu_monitor_json.c | 93 ++++++++++++++++++++++++++++++++++++<br>
 src/qemu/qemu_monitor_json.h |  4 ++<br>
 4 files changed, 161 insertions(+)<br>
<br>
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c<br>
index c2808c75a3..9581e90796 100644<br>
--- a/src/qemu/qemu_monitor.c<br>
+++ b/src/qemu/qemu_monitor.c<br>
@@ -4355,6 +4355,35 @@ qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type<br>
 }<br>
<br>
<br>
+VIR_ENUM_IMPL(qemuMonitorQueryStatsUnit,<br>
+              QEMU_MONITOR_QUERY_STATS_UNIT_LAST,<br>
+              "bytes",<br>
+              "seconds",<br>
+              "cycles",<br>
+              "boolean",<br>
+);<br>
+<br>
+<br>
+VIR_ENUM_IMPL(qemuMonitorQueryStatsType,<br>
+              QEMU_MONITOR_QUERY_STATS_TYPE_LAST,<br>
+              "cumulative",<br>
+              "instant",<br>
+              "peak",<br>
+              "linear-histogram",<br>
+              "log2-histogram",<br>
+);<br>
+<br>
+<br>
+GHashTable *<br>
+qemuMonitorQueryStatsSchema(qemuMonitor *mon,<br>
+                            qemuMonitorQueryStatsProviderType provider_type)<br>
+{<br>
+    QEMU_CHECK_MONITOR_NULL(mon);<br>
+<br>
+    return qemuMonitorJSONQueryStatsSchema(mon, provider_type);<br>
+}<br>
+<br>
+<br>
 virJSONValue *<br>
 qemuMonitorQueryStats(qemuMonitor *mon,<br>
                       qemuMonitorQueryStatsTargetType target,<br>
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h<br>
index 63269e15bc..4c817dea20 100644<br>
--- a/src/qemu/qemu_monitor.h<br>
+++ b/src/qemu/qemu_monitor.h<br>
@@ -1521,6 +1521,41 @@ qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type<br>
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuMonitorQueryStatsProvider,<br>
                               qemuMonitorQueryStatsProviderFree);<br>
<br>
+typedef enum {<br>
+    QEMU_MONITOR_QUERY_STATS_UNIT_BYTES,<br>
+    QEMU_MONITOR_QUERY_STATS_UNIT_SECONDS,<br>
+    QEMU_MONITOR_QUERY_STATS_UNIT_CYCLES,<br>
+    QEMU_MONITOR_QUERY_STATS_UNIT_BOOLEAN,<br>
+    QEMU_MONITOR_QUERY_STATS_UNIT_LAST,<br>
+} qemuMonitorQueryStatsUnitType;<br>
+<br>
+VIR_ENUM_DECL(qemuMonitorQueryStatsUnit);<br>
+<br>
+typedef enum {<br>
+    QEMU_MONITOR_QUERY_STATS_TYPE_CUMULATIVE,<br>
+    QEMU_MONITOR_QUERY_STATS_TYPE_INSTANT,<br>
+    QEMU_MONITOR_QUERY_STATS_TYPE_PEAK,<br>
+    QEMU_MONITOR_QUERY_STATS_TYPE_LINEAR_HISTOGRAM,<br>
+    QEMU_MONITOR_QUERY_STATS_TYPE_LOG2_HISTOGRAM,<br>
+    QEMU_MONITOR_QUERY_STATS_TYPE_LAST,<br>
+} qemuMonitorQueryStatsTypeType;<br>
+<br>
+VIR_ENUM_DECL(qemuMonitorQueryStatsType);<br>
+<br>
+typedef struct _qemuMonitorQueryStatsSchemaData qemuMonitorQueryStatsSchemaData;<br>
+struct _qemuMonitorQueryStatsSchemaData {<br>
+    qemuMonitorQueryStatsTargetType target;<br>
+    qemuMonitorQueryStatsUnitType unit;<br>
+    qemuMonitorQueryStatsTypeType type;<br>
+    unsigned int bucket_size;<br>
+    int base;<br>
+    int exponent;<br>
+};<br>
+<br>
+GHashTable *<br>
+qemuMonitorQueryStatsSchema(qemuMonitor *mon,<br>
+                            qemuMonitorQueryStatsProviderType provider_type);<br>
+<br>
 virJSONValue *<br>
 qemuMonitorQueryStats(qemuMonitor *mon,<br>
                       qemuMonitorQueryStatsTargetType target,<br>
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c<br>
index 70fba50e6c..f822a8908c 100644<br>
--- a/src/qemu/qemu_monitor_json.c<br>
+++ b/src/qemu/qemu_monitor_json.c<br>
@@ -8575,6 +8575,99 @@ qemuMonitorJSONMigrateRecover(qemuMonitor *mon,<br>
     return qemuMonitorJSONCheckError(cmd, reply);<br>
 }<br>
<br>
+static GHashTable *<br>
+qemuMonitorJSONExtractQueryStatsSchema(virJSONValue *json)<br>
+{<br>
+    g_autoptr(GHashTable) schema = virHashNew(g_free);<br>
+    size_t i;<br>
+<br>
+    for (i = 0; i < virJSONValueArraySize(json); i++) {<br>
+        virJSONValue *obj, *stats;<br>
+        const char *target_str;<br>
+        int target;<br>
+        size_t j;<br>
+<br>
+        obj = virJSONValueArrayGet(json, i);<br>
+<br>
+        if (!virJSONValueIsObject(obj))<br>
+            continue;<br>
+<br>
+        stats = virJSONValueObjectGetArray(obj, "stats");<br>
+<br>
+        if (!virJSONValueIsArray(stats))<br>
+            continue;<br>
+<br>
+        target_str = virJSONValueObjectGetString(obj, "target");<br>
+        target = qemuMonitorQueryStatsTargetTypeFromString(target_str);<br>
+<br>
+        for (j = 0; j < virJSONValueArraySize(stats); j++) {<br>
+            virJSONValue *stat = virJSONValueArrayGet(stats, j);<br>
+            const char *name, *type_str, *unit_str;<br>
+            qemuMonitorQueryStatsSchemaData *data;<br>
+            int type, unit;<br></blockquote><div><br></div><div>We try to declare each variable on a separate line, even when there is more of the same type.</div><div><br></div><div>In this case, I would also declare (and initialize) the variable 'stat' as the last, as its value is</div><div>checked right after its initialization.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+            if (!virJSONValueIsObject(stat))<br>
+                continue;<br>
+<br>
+            name = virJSONValueObjectGetString(stat, "name");<br>
+<br></blockquote><div><br></div><div>I prefer checks that look like:</div><div><br></div><div>    if (!(name = virJSONValueObjectGetString(stat, "name")))</div><div>        continue;</div><div><br></div><div>But that's just my personal preference and you can ignore this note if you do not like it.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+            if (!name)<br>
+                continue;<br>
+<br>
+            type_str = virJSONValueObjectGetString(stat, "type");<br>
+            unit_str = virJSONValueObjectGetString(stat, "unit");<br>
+            type = qemuMonitorQueryStatsTypeTypeFromString(type_str);<br>
+            unit = qemuMonitorQueryStatsUnitTypeFromString(unit_str);<br>
+<br>
+            data = g_new0(qemuMonitorQueryStatsSchemaData, 1);<br>
+            data->target = (target == -1) ? QEMU_MONITOR_QUERY_STATS_TARGET_LAST : target;<br>
+            data->type = (type == -1) ? QEMU_MONITOR_QUERY_STATS_TYPE_LAST : type;<br>
+            data->unit = (unit == -1) ? QEMU_MONITOR_QUERY_STATS_UNIT_LAST : unit;<br>
+<br>
+            if (virJSONValueObjectGetNumberInt(stat, "base", &data->base) < 0 ||<br>
+                virJSONValueObjectGetNumberInt(stat, "exponent", &data->exponent) < 0)<br>
+                data->base = 0, data->exponent = 0;<br></blockquote><div><br></div><div>Definitely split this line into two separate lines please. Operator comma makes the code very unclean</div><div>and should not be used aside from a few exceptions in 'for' and 'while' loops. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+            /* a base of zero means that there is simply no scale, data->exponent is<br>
+               set to 0 just for safety measures */<br>
+<br>
+            if (data->type == QEMU_MONITOR_QUERY_STATS_TYPE_LINEAR_HISTOGRAM &&<br>
+                virJSONValueObjectGetNumberUint(stat, "bucket-size", &data->bucket_size) < 0)<br>
+                data->bucket_size = 0;<br>
+<br>
+            virHashAddEntry(schema, name, data);<br>
+        }<br>
+    }<br>
+<br>
+    return g_steal_pointer(&schema);<br>
+}<br>
+<br>
+GHashTable *<br>
+qemuMonitorJSONQueryStatsSchema(qemuMonitor *mon,<br>
+                                qemuMonitorQueryStatsProviderType provider_type)<br>
+{<br>
+    g_autoptr(virJSONValue) cmd = NULL;<br>
+    g_autoptr(virJSONValue) reply = NULL;<br>
+    virJSONValue *ret;<br>
+<br>
+    const char *type_str = qemuMonitorQueryStatsProviderTypeToString(provider_type);<br>
+<br>
+    if (!(cmd = qemuMonitorJSONMakeCommand("query-stats-schemas",<br>
+                                           "S:provider", type_str,<br>
+                                           NULL)))</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+        return NULL;<br>
+<br>
+    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)<br>
+        return NULL;<br>
+<br>
+    if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_ARRAY) < 0)<br>
+        return NULL;<br>
+<br>
+    ret = virJSONValueObjectGetArray(reply, "return");<br>
+<br>
+    return qemuMonitorJSONExtractQueryStatsSchema(ret);<br>
+}<br>
+<br>
<br>
 /**<br>
  * qemuMonitorJSONQueryStats:<br>
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h<br>
index a53e6423df..c910e46504 100644<br>
--- a/src/qemu/qemu_monitor_json.h<br>
+++ b/src/qemu/qemu_monitor_json.h<br>
@@ -813,6 +813,10 @@ int<br>
 qemuMonitorJSONMigrateRecover(qemuMonitor *mon,<br>
                               const char *uri);<br>
<br>
+GHashTable *<br>
+qemuMonitorJSONQueryStatsSchema(qemuMonitor *mon,<br>
+                                qemuMonitorQueryStatsProviderType provider_type);<br>
+<br>
 virJSONValue *<br>
 qemuMonitorJSONQueryStats(qemuMonitor *mon,<br>
                           qemuMonitorQueryStatsTargetType target,<br>
-- <br>
2.37.1<br>
<br></blockquote><div><br></div><div>Otherwise nice:)</div><div><br></div><div>Kristina <br></div></div></div>