On Wed, Sep 07, 2022 at 02:23:24PM +0200, Kristina Hanicova wrote:
Hi, just a few notes on the code quality. On Wed, Sep 7, 2022 at 12:34 PM Amneesh Singh <natto@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> --- 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.
and you can even make it nicer with data->base = data->exponent = 0;
+ + /* 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.1Otherwise nice:) Kristina
Attachment:
signature.asc
Description: PGP signature
