[RFC PATCH 1/6] qemu_monitor: add qemuMonitorQueryStatsSchema
Martin Kletzander
mkletzan at redhat.com
Wed Sep 7 12:49:53 UTC 2022
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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220907/1d0489fb/attachment.sig>
More information about the libvir-list
mailing list