On 3/21/2022 9:55 AM, Paolo Bonzini wrote:
On 3/21/22 14:50, Markus Armbruster wrote:
Mark Kanda <mark.ka...@oracle.com> writes:
Thank you Markus.
On 3/11/2022 7:06 AM, Markus Armbruster wrote:
Are the stats bulky enough to justfify the extra complexity of
filtering?
If this was only for KVM, the complexity probably isn't worth it. However, the
framework is intended to support future stats with new providers and targets
(there has also been mention of moving existing stats to this framework).
Without some sort of filtering, I think the payload could become unmanageable.
I'm deeply wary of "may need $complexity in the future" when $complexity
could be added when we actually need it :)
I think it's better to have the filtering already. There are several uses for
it.
Regarding filtering by provider, consider that a command like "info jit"
should be a wrapper over
{ "execute": "query-stats", "arguments" : { "target": "vm",
"filters": [ { "provider": "tcg" } ] } }
So we have an example of the intended use already within QEMU. Yes, the
usefulness depends on actually having >1 provider but I think it's pretty
central to the idea of having a statistics *subsystem*.
Regarding filtering by name, query-stats mostly has two usecases. The first is
retrieving all stats and publishing them up to the user, for example once per
minute per VM. The second is monitoring a small number and building a
relatively continuous plot (e.g. 1-10 times per second per vCPU). For the
latter, not having to return hundreds of values unnecessarily (KVM has almost
60 stats, multiply by the number of vCPUs and the frequency) is worth having
even just with the KVM provider.
Can you give a use case for query-stats-schemas?
'query-stats-schemas' provide the the type details about each stat; such as the
unit, base, etc. These details are not reported by 'query-stats' (only the stat
name and raw values are returned).
Yes, but what is going to use these type details, and for what purpose?
QEMU does not know in advance which stats are provided. The types, etc. are
provided by the kernel and can change by architecture and kernel version. In
the case of KVM, introspection is done through a file descriptor. QEMU passes
these up as QMP and in the future it could/should extend this to other
providers (such as TCG) and devices (such as block devices).
See the "info stats" implementation for how it uses the schema:
vcpu (qom path: /machine/unattached/device[2])
provider: kvm
exits (cumulative): 52369
halt_wait_ns (cumulative nanoseconds): 416092704390
Information such as "cumulative nanoseconds" is provided by the schema.
Have you considered splitting this up into three parts: unfiltered
query-stats, filtering, and query-stats-schemas?
Splitting could be an idea, but I think only filtering would be a separate
step. The stats are not really usable without a schema that tells you the
units, or whether a number can go down or only up. (Well, a human export
could use them through its intuition, but a HMP-level command could not be
provided).
We could perhaps merge with the current schema, then clean it up on top,
both in 7.1, if that's easier for you.
The serialized JSON would change, so that would be a bit worrisome (but it
makes me feel a little less bad about this missing 7.0). It seems to be as
easy as this, as far as alternates go:
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 3cb389e875..48578e1698 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -554,7 +554,7 @@ def check_alternate(expr: _JSONObject, info:
QAPISourceInfo) -> None:
check_name_lower(key, info, source)
check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, source)
- check_type(value['type'], info, source)
+ check_type(value['type'], info, source, allow_array=True)
def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b7b3fc0ce4..3728340c37 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -243,6 +243,7 @@ def alternate_qtype(self):
'number': 'QTYPE_QNUM',
'int': 'QTYPE_QNUM',
'boolean': 'QTYPE_QBOOL',
+ 'array': 'QTYPE_QLIST',
'object': 'QTYPE_QDICT'
}
return json2qtype.get(self.json_type())
@@ -1069,6 +1070,9 @@ def _def_struct_type(self, expr, info, doc):
None))
def _make_variant(self, case, typ, ifcond, info):
+ if isinstance(typ, list):
+ assert len(typ) == 1
+ typ = self._make_array_type(typ[0], info)
return QAPISchemaVariant(case, info, typ, ifcond)
def _def_union_type(self, expr, info, doc):
I'll try to write some testcases and also cover other uses of
_make_variant, which will undoubtedly find some issue.
Hi Paolo,
FWIW, the attached patch adjusts some tests for alternates with arrays..
Thanks/regards,
-Mark
From 8d02b1b3cbb0dcc08875e307199d06c3995b3cf2 Mon Sep 17 00:00:00 2001
From: Mark Kanda <mark.ka...@oracle.com>
Date: Tue, 15 Mar 2022 20:42:05 -0500
Subject: [PATCH] qapi: Add support for alternates with arrays
Signed-off-by: Mark Kanda <mark.ka...@oracle.com>
---
scripts/qapi/expr.py | 2 +-
scripts/qapi/schema.py | 6 +++++-
tests/qapi-schema/alternate-array.err | 2 --
tests/qapi-schema/alternate-array.json | 7 -------
tests/qapi-schema/alternate-array.out | 0
tests/qapi-schema/meson.build | 1 -
tests/qapi-schema/qapi-schema-test.json | 6 ++++++
tests/qapi-schema/qapi-schema-test.out | 8 ++++++++
8 files changed, 20 insertions(+), 12 deletions(-)
delete mode 100644 tests/qapi-schema/alternate-array.err
delete mode 100644 tests/qapi-schema/alternate-array.json
delete mode 100644 tests/qapi-schema/alternate-array.out
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 3cb389e875..48578e1698 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -554,7 +554,7 @@ def check_alternate(expr: _JSONObject, info:
QAPISourceInfo) -> None:
check_name_lower(key, info, source)
check_keys(value, info, source, ['type'], ['if'])
check_if(value, info, source)
- check_type(value['type'], info, source)
+ check_type(value['type'], info, source, allow_array=True)
def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b7b3fc0ce4..7eedfa6cc2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -243,7 +243,8 @@ def alternate_qtype(self):
'number': 'QTYPE_QNUM',
'int': 'QTYPE_QNUM',
'boolean': 'QTYPE_QBOOL',
- 'object': 'QTYPE_QDICT'
+ 'object': 'QTYPE_QDICT',
+ 'array': 'QTYPE_QLIST'
}
return json2qtype.get(self.json_type())
@@ -1069,6 +1070,9 @@ def _def_struct_type(self, expr, info, doc):
None))
def _make_variant(self, case, typ, ifcond, info):
+ if isinstance(typ, list):
+ assert len(typ) == 1
+ typ = self._make_array_type(typ[0], info)
return QAPISchemaVariant(case, info, typ, ifcond)
def _def_union_type(self, expr, info, doc):
diff --git a/tests/qapi-schema/alternate-array.err
b/tests/qapi-schema/alternate-array.err
deleted file mode 100644
index b1aa1f4e8d..0000000000
--- a/tests/qapi-schema/alternate-array.err
+++ /dev/null
@@ -1,2 +0,0 @@
-alternate-array.json: In alternate 'Alt':
-alternate-array.json:5: 'data' member 'two' cannot be an array
diff --git a/tests/qapi-schema/alternate-array.json
b/tests/qapi-schema/alternate-array.json
deleted file mode 100644
index f241aac122..0000000000
--- a/tests/qapi-schema/alternate-array.json
+++ /dev/null
@@ -1,7 +0,0 @@
-# we do not allow array branches in alternates
-# TODO: should we support this?
-{ 'struct': 'One',
- 'data': { 'name': 'str' } }
-{ 'alternate': 'Alt',
- 'data': { 'one': 'One',
- 'two': [ 'int' ] } }
diff --git a/tests/qapi-schema/alternate-array.out
b/tests/qapi-schema/alternate-array.out
deleted file mode 100644
index e69de29bb2..0000000000
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index caf0791ba8..3dbd5f8bfb 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -4,7 +4,6 @@ test_env.set('PYTHONIOENCODING', 'utf-8')
schemas = [
'alternate-any.json',
- 'alternate-array.json',
'alternate-base.json',
'alternate-branch-if-invalid.json',
'alternate-clash.json',
diff --git a/tests/qapi-schema/qapi-schema-test.json
b/tests/qapi-schema/qapi-schema-test.json
index 43b8697002..64dcbbbe2a 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -123,6 +123,12 @@
# for testing use of 'str' within alternates
{ 'alternate': 'AltStrObj', 'data': { 's': 'str', 'o': 'TestStruct' } }
+# for testing use of an 'int' array within alternates
+{ 'alternate': 'AltIntArray', 'data': { 'a': [ 'int' ], 'o': 'TestStruct' } }
+
+# for testing use of an 'TestStruct' array within alternates
+{ 'alternate': 'AltStructArray', 'data': { 'a': [ 'TestStruct' ], 'o':
'TestStruct' } }
+
{ 'struct': 'ArrayStruct',
'data': { 'integer': ['int'],
's8': ['int8'],
diff --git a/tests/qapi-schema/qapi-schema-test.out
b/tests/qapi-schema/qapi-schema-test.out
index 1f9585fa9b..b899c30158 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -125,6 +125,14 @@ alternate AltStrObj
tag type
case s: str
case o: TestStruct
+alternate AltIntArray
+ tag type
+ case a: intList
+ case o: TestStruct
+alternate AltStructArray
+ tag type
+ case a: TestStructList
+ case o: TestStruct
object ArrayStruct
member integer: intList optional=False
member s8: int8List optional=False
--
2.27.0
