Hi, just a few notes on the code quality.
On Wed, Sep 7, 2022 at 12:34 PM Amneesh Singh <na...@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 <na...@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