7:49 a.m.
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(a)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(a)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.
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.1
>
>
Otherwise nice:)
Kristina