Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/unit/test-qobject-input-visitor.c | 218 ++++++++++++++++++
tests/qapi-schema/alias-bad-type.err | 2 +
tests/qapi-schema/alias-bad-type.json | 3 +
tests/qapi-schema/alias-bad-type.out | 0
tests/qapi-schema/alias-missing-source.err | 2 +
tests/qapi-schema/alias-missing-source.json | 3 +
tests/qapi-schema/alias-missing-source.out | 0
tests/qapi-schema/alias-name-bad-type.err | 2 +
tests/qapi-schema/alias-name-bad-type.json | 3 +
tests/qapi-schema/alias-name-bad-type.out | 0
tests/qapi-schema/alias-name-conflict.err | 2 +
tests/qapi-schema/alias-name-conflict.json | 4 +
tests/qapi-schema/alias-name-conflict.out | 0
tests/qapi-schema/alias-recursive.err | 2 +
tests/qapi-schema/alias-recursive.json | 4 +
tests/qapi-schema/alias-recursive.out | 0
tests/qapi-schema/alias-source-bad-type.err | 2 +
tests/qapi-schema/alias-source-bad-type.json | 3 +
tests/qapi-schema/alias-source-bad-type.out | 0
.../alias-source-elem-bad-type.err | 2 +
.../alias-source-elem-bad-type.json | 3 +
.../alias-source-elem-bad-type.out | 0
tests/qapi-schema/alias-source-empty.err | 2 +
tests/qapi-schema/alias-source-empty.json | 3 +
tests/qapi-schema/alias-source-empty.out | 0
.../alias-source-inexistent-variants.err | 2 +
.../alias-source-inexistent-variants.json | 12 +
.../alias-source-inexistent-variants.out | 0
tests/qapi-schema/alias-source-inexistent.err | 2 +
.../qapi-schema/alias-source-inexistent.json | 3 +
tests/qapi-schema/alias-source-inexistent.out | 0
.../alias-source-non-object-path.err | 2 +
.../alias-source-non-object-path.json | 3 +
.../alias-source-non-object-path.out | 0
.../alias-source-non-object-wildcard.err | 2 +
.../alias-source-non-object-wildcard.json | 3 +
.../alias-source-non-object-wildcard.out | 0
...lias-source-optional-wildcard-indirect.err | 2 +
...ias-source-optional-wildcard-indirect.json | 6 +
...lias-source-optional-wildcard-indirect.out | 0
.../alias-source-optional-wildcard.err | 2 +
.../alias-source-optional-wildcard.json | 5 +
.../alias-source-optional-wildcard.out | 0
tests/qapi-schema/alias-unknown-key.err | 3 +
tests/qapi-schema/alias-unknown-key.json | 3 +
tests/qapi-schema/alias-unknown-key.out | 0
tests/qapi-schema/aliases-bad-type.err | 2 +
tests/qapi-schema/aliases-bad-type.json | 3 +
tests/qapi-schema/aliases-bad-type.out | 0
tests/qapi-schema/meson.build | 16 ++
tests/qapi-schema/qapi-schema-test.json | 26 +++
tests/qapi-schema/qapi-schema-test.out | 31 +++
52 files changed, 388 insertions(+)
create mode 100644 tests/qapi-schema/alias-bad-type.err
create mode 100644 tests/qapi-schema/alias-bad-type.json
create mode 100644 tests/qapi-schema/alias-bad-type.out
create mode 100644 tests/qapi-schema/alias-missing-source.err
create mode 100644 tests/qapi-schema/alias-missing-source.json
create mode 100644 tests/qapi-schema/alias-missing-source.out
create mode 100644 tests/qapi-schema/alias-name-bad-type.err
create mode 100644 tests/qapi-schema/alias-name-bad-type.json
create mode 100644 tests/qapi-schema/alias-name-bad-type.out
create mode 100644 tests/qapi-schema/alias-name-conflict.err
create mode 100644 tests/qapi-schema/alias-name-conflict.json
create mode 100644 tests/qapi-schema/alias-name-conflict.out
create mode 100644 tests/qapi-schema/alias-recursive.err
create mode 100644 tests/qapi-schema/alias-recursive.json
create mode 100644 tests/qapi-schema/alias-recursive.out
create mode 100644 tests/qapi-schema/alias-source-bad-type.err
create mode 100644 tests/qapi-schema/alias-source-bad-type.json
create mode 100644 tests/qapi-schema/alias-source-bad-type.out
create mode 100644 tests/qapi-schema/alias-source-elem-bad-type.err
create mode 100644 tests/qapi-schema/alias-source-elem-bad-type.json
create mode 100644 tests/qapi-schema/alias-source-elem-bad-type.out
create mode 100644 tests/qapi-schema/alias-source-empty.err
create mode 100644 tests/qapi-schema/alias-source-empty.json
create mode 100644 tests/qapi-schema/alias-source-empty.out
create mode 100644 tests/qapi-schema/alias-source-inexistent-variants.err
create mode 100644 tests/qapi-schema/alias-source-inexistent-variants.json
create mode 100644 tests/qapi-schema/alias-source-inexistent-variants.out
create mode 100644 tests/qapi-schema/alias-source-inexistent.err
create mode 100644 tests/qapi-schema/alias-source-inexistent.json
create mode 100644 tests/qapi-schema/alias-source-inexistent.out
create mode 100644 tests/qapi-schema/alias-source-non-object-path.err
create mode 100644 tests/qapi-schema/alias-source-non-object-path.json
create mode 100644 tests/qapi-schema/alias-source-non-object-path.out
create mode 100644 tests/qapi-schema/alias-source-non-object-wildcard.err
create mode 100644 tests/qapi-schema/alias-source-non-object-wildcard.json
create mode 100644 tests/qapi-schema/alias-source-non-object-wildcard.out
create mode 100644 tests/qapi-schema/alias-source-optional-wildcard-indirect.err
create mode 100644 tests/qapi-schema/alias-source-optional-wildcard-indirect.json
create mode 100644 tests/qapi-schema/alias-source-optional-wildcard-indirect.out
create mode 100644 tests/qapi-schema/alias-source-optional-wildcard.err
create mode 100644 tests/qapi-schema/alias-source-optional-wildcard.json
create mode 100644 tests/qapi-schema/alias-source-optional-wildcard.out
create mode 100644 tests/qapi-schema/alias-unknown-key.err
create mode 100644 tests/qapi-schema/alias-unknown-key.json
create mode 100644 tests/qapi-schema/alias-unknown-key.out
create mode 100644 tests/qapi-schema/aliases-bad-type.err
create mode 100644 tests/qapi-schema/aliases-bad-type.json
create mode 100644 tests/qapi-schema/aliases-bad-type.out
diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
index e41b91a2a6..f2891b6f5d 100644
--- a/tests/unit/test-qobject-input-visitor.c
+++ b/tests/unit/test-qobject-input-visitor.c
@@ -952,6 +952,214 @@ static void test_visitor_in_list_union_number(TestInputVisitorData *data,
g_string_free(gstr_list, true);
}
+static void test_visitor_in_alias_struct_local(TestInputVisitorData *data,
+ const void *unused)
+{
+ AliasStruct1 *tmp = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ /* Can still specify the real member name with alias support */
+ v = visitor_input_test_init(data, "{ 'foo': 42 }");
+ visit_type_AliasStruct1(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->foo, ==, 42);
+ qapi_free_AliasStruct1(tmp);
+
+ /* The alias is a working alternative */
+ v = visitor_input_test_init(data, "{ 'bar': 42 }");
+ visit_type_AliasStruct1(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->foo, ==, 42);
+ qapi_free_AliasStruct1(tmp);
+
+ /* But you can't use both at the same time */
+ v = visitor_input_test_init(data, "{ 'foo': 5, 'bar': 42 }");
+ visit_type_AliasStruct1(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+}
+
+static void test_visitor_in_alias_struct_nested(TestInputVisitorData *data,
+ const void *unused)
+{
+ AliasStruct2 *tmp = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ /* Can still specify the real member names with alias support */
+ v = visitor_input_test_init(data, "{ 'nested': { 'foo': 42 } }");
+ visit_type_AliasStruct2(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->nested->foo, ==, 42);
+ qapi_free_AliasStruct2(tmp);
+
+ /* The inner alias is a working alternative */
+ v = visitor_input_test_init(data, "{ 'nested': { 'bar': 42 } }");
+ visit_type_AliasStruct2(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->nested->foo, ==, 42);
+ qapi_free_AliasStruct2(tmp);
+
+ /* So is the outer alias */
+ v = visitor_input_test_init(data, "{ 'bar': 42 }");
+ visit_type_AliasStruct2(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->nested->foo, ==, 42);
+ qapi_free_AliasStruct2(tmp);
+
+ /* You can't use more than one option at the same time */
+ v = visitor_input_test_init(data, "{ 'bar': 5, 'nested': { 'foo': 42 } }");
+ visit_type_AliasStruct2(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+
+ v = visitor_input_test_init(data, "{ 'bar': 5, 'nested': { 'bar': 42 } }");
+ visit_type_AliasStruct2(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+
+ v = visitor_input_test_init(data, "{ 'nested': { 'foo': 42, 'bar': 42 } }");
+ visit_type_AliasStruct2(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+
+ v = visitor_input_test_init(data, "{ 'bar': 5, "
+ " 'nested': { 'foo': 42, 'bar': 42 } }");
+ visit_type_AliasStruct2(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+}
+
+static void test_visitor_in_alias_wildcard(TestInputVisitorData *data,
+ const void *unused)
+{
+ AliasStruct3 *tmp = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ /* Can still specify the real member names with alias support */
+ v = visitor_input_test_init(data, "{ 'nested': { 'foo': 42 } }");
+ visit_type_AliasStruct3(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->nested->foo, ==, 42);
+ qapi_free_AliasStruct3(tmp);
+
+ /* The wildcard alias makes it work on the top level */
+ v = visitor_input_test_init(data, "{ 'foo': 42 }");
+ visit_type_AliasStruct3(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->nested->foo, ==, 42);
+ qapi_free_AliasStruct3(tmp);
+
+ /* It makes the inner alias available, too */
+ v = visitor_input_test_init(data, "{ 'bar': 42 }");
+ visit_type_AliasStruct3(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->nested->foo, ==, 42);
+ qapi_free_AliasStruct3(tmp);
+
+ /* You can't use more than one option at the same time */
+ v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'foo': 42 } }");
+ visit_type_AliasStruct3(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+
+ v = visitor_input_test_init(data, "{ 'bar': 42, 'nested': { 'foo': 42 } }");
+ visit_type_AliasStruct3(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+
+ v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'bar': 42 } }");
+ visit_type_AliasStruct3(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+
+ v = visitor_input_test_init(data, "{ 'bar': 42, 'nested': { 'bar': 42 } }");
+ visit_type_AliasStruct3(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+
+ v = visitor_input_test_init(data, "{ 'foo': 42, 'bar': 42 }");
+ visit_type_AliasStruct3(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+}
+
+static void test_visitor_in_alias_flat_union(TestInputVisitorData *data,
+ const void *unused)
+{
+ AliasFlatUnion *tmp = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ /* Can still specify the real member name with alias support */
+ v = visitor_input_test_init(data, "{ 'tag': 'drei' }");
+ visit_type_AliasFlatUnion(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->tag, ==, FEATURE_ENUM1_DREI);
+ qapi_free_AliasFlatUnion(tmp);
+
+ /* Use alias for a base member (the discriminator even) */
+ v = visitor_input_test_init(data, "{ 'variant': 'zwei' }");
+ visit_type_AliasFlatUnion(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->tag, ==, FEATURE_ENUM1_ZWEI);
+ qapi_free_AliasFlatUnion(tmp);
+
+ /* Use alias for a variant member */
+ v = visitor_input_test_init(data, "{ 'tag': 'eins', 'bar': 42 }");
+ visit_type_AliasFlatUnion(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->tag, ==, FEATURE_ENUM1_EINS);
+ g_assert_cmpint(tmp->u.eins.foo, ==, 42);
+ qapi_free_AliasFlatUnion(tmp);
+
+ /* Both together */
+ v = visitor_input_test_init(data, "{ 'variant': 'eins', 'bar': 42 }");
+ visit_type_AliasFlatUnion(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->tag, ==, FEATURE_ENUM1_EINS);
+ g_assert_cmpint(tmp->u.eins.foo, ==, 42);
+ qapi_free_AliasFlatUnion(tmp);
+
+ /* You can't use more than one option at the same time for each alias */
+ v = visitor_input_test_init(data, "{ 'variant': 'zwei', 'tag': 'drei' }");
+ visit_type_AliasFlatUnion(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+
+ v = visitor_input_test_init(data, "{ 'tag': 'eins', 'foo': 6, 'bar': 9 }");
+ visit_type_AliasFlatUnion(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+}
+
+static void test_visitor_in_alias_simple_union(TestInputVisitorData *data,
+ const void *unused)
+{
+ AliasSimpleUnion *tmp = NULL;
+ Error *err = NULL;
+ Visitor *v;
+
+ /* Can still specify the real member name with alias support */
+ v = visitor_input_test_init(data, "{ 'type': 'eins', "
+ " 'data': { 'foo': 42 } }");
+ visit_type_AliasSimpleUnion(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->type, ==, ALIAS_SIMPLE_UNION_KIND_EINS);
+ g_assert_cmpint(tmp->u.eins.data->foo, ==, 42);
+ qapi_free_AliasSimpleUnion(tmp);
+
+ /* 'type' can be aliased */
+ v = visitor_input_test_init(data, "{ 'tag': 'eins', "
+ " 'data': { 'foo': 42 } }");
+ visit_type_AliasSimpleUnion(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->type, ==, ALIAS_SIMPLE_UNION_KIND_EINS);
+ g_assert_cmpint(tmp->u.eins.data->foo, ==, 42);
+ qapi_free_AliasSimpleUnion(tmp);
+
+ /* The wildcard alias makes it work on the top level */
+ v = visitor_input_test_init(data, "{ 'type': 'eins', 'foo': 42 }");
+ visit_type_AliasSimpleUnion(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->type, ==, ALIAS_SIMPLE_UNION_KIND_EINS);
+ g_assert_cmpint(tmp->u.eins.data->foo, ==, 42);
+ qapi_free_AliasSimpleUnion(tmp);
+
+ /* It makes the inner alias available, too */
+ v = visitor_input_test_init(data, "{ 'type': 'eins', 'bar': 42 }");
+ visit_type_AliasSimpleUnion(v, NULL, &tmp, &error_abort);
+ g_assert_cmpint(tmp->type, ==, ALIAS_SIMPLE_UNION_KIND_EINS);
+ g_assert_cmpint(tmp->u.eins.data->foo, ==, 42);
+ qapi_free_AliasSimpleUnion(tmp);
+
+ /* You can't use more than one option at the same time for each alias */
+ v = visitor_input_test_init(data, "{ 'type': 'eins', 'tag': 'eins' }");
+ visit_type_AliasSimpleUnion(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+
+ v = visitor_input_test_init(data, "{ 'type': 'eins', "
+ " 'bar': 123, "
+ " 'data': { 'foo': 312 } }");
+ visit_type_AliasSimpleUnion(v, NULL, &tmp, &err);
+ error_free_or_abort(&err);
+}
+
static void input_visitor_test_add(const char *testpath,
const void *user_data,
void (*test_func)(TestInputVisitorData *data,
@@ -1350,6 +1558,16 @@ int main(int argc, char **argv)
NULL, test_visitor_in_list_union_string);
input_visitor_test_add("/visitor/input/list_union/number",
NULL, test_visitor_in_list_union_number);
+ input_visitor_test_add("/visitor/input/alias/struct-local",
+ NULL, test_visitor_in_alias_struct_local);
+ input_visitor_test_add("/visitor/input/alias/struct-nested",
+ NULL, test_visitor_in_alias_struct_nested);
+ input_visitor_test_add("/visitor/input/alias/wildcard",
+ NULL, test_visitor_in_alias_wildcard);
+ input_visitor_test_add("/visitor/input/alias/flat-union",
+ NULL, test_visitor_in_alias_flat_union);
+ input_visitor_test_add("/visitor/input/alias/simple-union",
+ NULL, test_visitor_in_alias_simple_union);
input_visitor_test_add("/visitor/input/fail/struct",
NULL, test_visitor_in_fail_struct);
input_visitor_test_add("/visitor/input/fail/struct-nested",
diff --git a/tests/qapi-schema/alias-bad-type.err b/tests/qapi-schema/alias-bad-type.err
new file mode 100644
index 0000000000..820e18ed9c
--- /dev/null
+++ b/tests/qapi-schema/alias-bad-type.err
@@ -0,0 +1,2 @@
+alias-bad-type.json: In struct 'AliasStruct0':
+alias-bad-type.json:1: 'aliases' members must be objects
diff --git a/tests/qapi-schema/alias-bad-type.json b/tests/qapi-schema/alias-bad-type.json
new file mode 100644
index 0000000000..0aa5d206fe
--- /dev/null
+++ b/tests/qapi-schema/alias-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ 'must be an object' ] }
diff --git a/tests/qapi-schema/alias-bad-type.out b/tests/qapi-schema/alias-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-missing-source.err b/tests/qapi-schema/alias-missing-source.err
new file mode 100644
index 0000000000..8b7d601fbf
--- /dev/null
+++ b/tests/qapi-schema/alias-missing-source.err
@@ -0,0 +1,2 @@
+alias-missing-source.json: In struct 'AliasStruct0':
+alias-missing-source.json:1: 'aliases' member misses key 'source'
diff --git a/tests/qapi-schema/alias-missing-source.json b/tests/qapi-schema/alias-missing-source.json
new file mode 100644
index 0000000000..b6c91a9488
--- /dev/null
+++ b/tests/qapi-schema/alias-missing-source.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar' } ] }
diff --git a/tests/qapi-schema/alias-missing-source.out b/tests/qapi-schema/alias-missing-source.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-name-bad-type.err b/tests/qapi-schema/alias-name-bad-type.err
new file mode 100644
index 0000000000..489f45ff9b
--- /dev/null
+++ b/tests/qapi-schema/alias-name-bad-type.err
@@ -0,0 +1,2 @@
+alias-name-bad-type.json: In struct 'AliasStruct0':
+alias-name-bad-type.json:1: alias member 'name' requires a string name
diff --git a/tests/qapi-schema/alias-name-bad-type.json b/tests/qapi-schema/alias-name-bad-type.json
new file mode 100644
index 0000000000..17442d5939
--- /dev/null
+++ b/tests/qapi-schema/alias-name-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': ['bar'], 'source': ['foo'] } ] }
diff --git a/tests/qapi-schema/alias-name-bad-type.out b/tests/qapi-schema/alias-name-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-name-conflict.err b/tests/qapi-schema/alias-name-conflict.err
new file mode 100644
index 0000000000..d5825a0285
--- /dev/null
+++ b/tests/qapi-schema/alias-name-conflict.err
@@ -0,0 +1,2 @@
+alias-name-conflict.json: In struct 'AliasStruct0':
+alias-name-conflict.json:1: alias 'bar' collides with member 'bar'
diff --git a/tests/qapi-schema/alias-name-conflict.json b/tests/qapi-schema/alias-name-conflict.json
new file mode 100644
index 0000000000..bdb5bd4eab
--- /dev/null
+++ b/tests/qapi-schema/alias-name-conflict.json
@@ -0,0 +1,4 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int',
+ 'bar': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': ['foo'] } ] }
diff --git a/tests/qapi-schema/alias-name-conflict.out b/tests/qapi-schema/alias-name-conflict.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-recursive.err b/tests/qapi-schema/alias-recursive.err
new file mode 100644
index 0000000000..127ce019a8
--- /dev/null
+++ b/tests/qapi-schema/alias-recursive.err
@@ -0,0 +1,2 @@
+alias-recursive.json: In struct 'AliasStruct0':
+alias-recursive.json:1: alias 'baz' resolving to 'bar' makes 'bar' an alias for itself
diff --git a/tests/qapi-schema/alias-recursive.json b/tests/qapi-schema/alias-recursive.json
new file mode 100644
index 0000000000..e25b935324
--- /dev/null
+++ b/tests/qapi-schema/alias-recursive.json
@@ -0,0 +1,4 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': ['baz'] },
+ { 'name': 'baz', 'source': ['bar'] } ] }
diff --git a/tests/qapi-schema/alias-recursive.out b/tests/qapi-schema/alias-recursive.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-bad-type.err b/tests/qapi-schema/alias-source-bad-type.err
new file mode 100644
index 0000000000..b1779cbb8e
--- /dev/null
+++ b/tests/qapi-schema/alias-source-bad-type.err
@@ -0,0 +1,2 @@
+alias-source-bad-type.json: In struct 'AliasStruct0':
+alias-source-bad-type.json:1: alias member 'source' must be an array
diff --git a/tests/qapi-schema/alias-source-bad-type.json b/tests/qapi-schema/alias-source-bad-type.json
new file mode 100644
index 0000000000..d6a7430ee3
--- /dev/null
+++ b/tests/qapi-schema/alias-source-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': 'foo' } ] }
diff --git a/tests/qapi-schema/alias-source-bad-type.out b/tests/qapi-schema/alias-source-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-elem-bad-type.err b/tests/qapi-schema/alias-source-elem-bad-type.err
new file mode 100644
index 0000000000..f73fbece77
--- /dev/null
+++ b/tests/qapi-schema/alias-source-elem-bad-type.err
@@ -0,0 +1,2 @@
+alias-source-elem-bad-type.json: In struct 'AliasStruct0':
+alias-source-elem-bad-type.json:1: member of alias member 'source' requires a string name
diff --git a/tests/qapi-schema/alias-source-elem-bad-type.json b/tests/qapi-schema/alias-source-elem-bad-type.json
new file mode 100644
index 0000000000..1d08f56492
--- /dev/null
+++ b/tests/qapi-schema/alias-source-elem-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': ['foo', true] } ] }
diff --git a/tests/qapi-schema/alias-source-elem-bad-type.out b/tests/qapi-schema/alias-source-elem-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-empty.err b/tests/qapi-schema/alias-source-empty.err
new file mode 100644
index 0000000000..2848e762cb
--- /dev/null
+++ b/tests/qapi-schema/alias-source-empty.err
@@ -0,0 +1,2 @@
+alias-source-empty.json: In struct 'AliasStruct0':
+alias-source-empty.json:1: alias member 'source' must not be empty
diff --git a/tests/qapi-schema/alias-source-empty.json b/tests/qapi-schema/alias-source-empty.json
new file mode 100644
index 0000000000..74b529de4a
--- /dev/null
+++ b/tests/qapi-schema/alias-source-empty.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': [] } ] }
diff --git a/tests/qapi-schema/alias-source-empty.out b/tests/qapi-schema/alias-source-empty.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-inexistent-variants.err b/tests/qapi-schema/alias-source-inexistent-variants.err
new file mode 100644
index 0000000000..a5d4a4c334
--- /dev/null
+++ b/tests/qapi-schema/alias-source-inexistent-variants.err
@@ -0,0 +1,2 @@
+alias-source-inexistent-variants.json: In union 'AliasStruct0':
+alias-source-inexistent-variants.json:7: alias 'test' has a source path that does not exist in any variant of union type 'AliasStruct0'
diff --git a/tests/qapi-schema/alias-source-inexistent-variants.json b/tests/qapi-schema/alias-source-inexistent-variants.json
new file mode 100644
index 0000000000..6328095b86
--- /dev/null
+++ b/tests/qapi-schema/alias-source-inexistent-variants.json
@@ -0,0 +1,12 @@
+{ 'enum': 'Variants',
+ 'data': [ 'a', 'b' ] }
+{ 'struct': 'Variant0',
+ 'data': { 'foo': 'int' } }
+{ 'struct': 'Variant1',
+ 'data': { 'bar': 'int' } }
+{ 'union': 'AliasStruct0',
+ 'base': { 'type': 'Variants' },
+ 'discriminator': 'type',
+ 'data': { 'a': 'Variant0',
+ 'b': 'Variant1' },
+ 'aliases': [ { 'name': 'test', 'source': ['baz'] } ] }
diff --git a/tests/qapi-schema/alias-source-inexistent-variants.out b/tests/qapi-schema/alias-source-inexistent-variants.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-inexistent.err b/tests/qapi-schema/alias-source-inexistent.err
new file mode 100644
index 0000000000..2d65d3f588
--- /dev/null
+++ b/tests/qapi-schema/alias-source-inexistent.err
@@ -0,0 +1,2 @@
+alias-source-inexistent.json: In struct 'AliasStruct0':
+alias-source-inexistent.json:1: alias 'bar' has inexistent source
diff --git a/tests/qapi-schema/alias-source-inexistent.json b/tests/qapi-schema/alias-source-inexistent.json
new file mode 100644
index 0000000000..5106d3609f
--- /dev/null
+++ b/tests/qapi-schema/alias-source-inexistent.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': ['baz'] } ] }
diff --git a/tests/qapi-schema/alias-source-inexistent.out b/tests/qapi-schema/alias-source-inexistent.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-non-object-path.err b/tests/qapi-schema/alias-source-non-object-path.err
new file mode 100644
index 0000000000..b3c748350f
--- /dev/null
+++ b/tests/qapi-schema/alias-source-non-object-path.err
@@ -0,0 +1,2 @@
+alias-source-non-object-path.json: In struct 'AliasStruct0':
+alias-source-non-object-path.json:1: alias 'bar' has non-object 'foo' in its source path
diff --git a/tests/qapi-schema/alias-source-non-object-path.json b/tests/qapi-schema/alias-source-non-object-path.json
new file mode 100644
index 0000000000..808a3e6281
--- /dev/null
+++ b/tests/qapi-schema/alias-source-non-object-path.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': ['foo', 'baz'] } ] }
diff --git a/tests/qapi-schema/alias-source-non-object-path.out b/tests/qapi-schema/alias-source-non-object-path.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-non-object-wildcard.err b/tests/qapi-schema/alias-source-non-object-wildcard.err
new file mode 100644
index 0000000000..4adc0d2281
--- /dev/null
+++ b/tests/qapi-schema/alias-source-non-object-wildcard.err
@@ -0,0 +1,2 @@
+alias-source-non-object-wildcard.json: In struct 'AliasStruct0':
+alias-source-non-object-wildcard.json:1: wildcard alias has non-object 'foo' in its source path
diff --git a/tests/qapi-schema/alias-source-non-object-wildcard.json b/tests/qapi-schema/alias-source-non-object-wildcard.json
new file mode 100644
index 0000000000..59ce1081ef
--- /dev/null
+++ b/tests/qapi-schema/alias-source-non-object-wildcard.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'source': ['foo'] } ] }
diff --git a/tests/qapi-schema/alias-source-non-object-wildcard.out b/tests/qapi-schema/alias-source-non-object-wildcard.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-optional-wildcard-indirect.err b/tests/qapi-schema/alias-source-optional-wildcard-indirect.err
new file mode 100644
index 0000000000..b58b8ff00f
--- /dev/null
+++ b/tests/qapi-schema/alias-source-optional-wildcard-indirect.err
@@ -0,0 +1,2 @@
+alias-source-optional-wildcard-indirect.json: In struct 'AliasStruct0':
+alias-source-optional-wildcard-indirect.json:3: wildcard alias has optional object member 'nested' in its source path
diff --git a/tests/qapi-schema/alias-source-optional-wildcard-indirect.json b/tests/qapi-schema/alias-source-optional-wildcard-indirect.json
new file mode 100644
index 0000000000..fcf04969dc
--- /dev/null
+++ b/tests/qapi-schema/alias-source-optional-wildcard-indirect.json
@@ -0,0 +1,6 @@
+{ 'struct': 'Nested',
+ 'data': { 'foo': 'int' } }
+{ 'struct': 'AliasStruct0',
+ 'data': { '*nested': 'Nested' },
+ 'aliases': [ { 'name': 'nested-alias', 'source': ['nested'] },
+ { 'source': ['nested-alias'] } ] }
diff --git a/tests/qapi-schema/alias-source-optional-wildcard-indirect.out b/tests/qapi-schema/alias-source-optional-wildcard-indirect.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-optional-wildcard.err b/tests/qapi-schema/alias-source-optional-wildcard.err
new file mode 100644
index 0000000000..e39200bd3d
--- /dev/null
+++ b/tests/qapi-schema/alias-source-optional-wildcard.err
@@ -0,0 +1,2 @@
+alias-source-optional-wildcard.json: In struct 'AliasStruct0':
+alias-source-optional-wildcard.json:3: wildcard alias has optional object member 'nested' in its source path
diff --git a/tests/qapi-schema/alias-source-optional-wildcard.json b/tests/qapi-schema/alias-source-optional-wildcard.json
new file mode 100644
index 0000000000..1a315f2ae0
--- /dev/null
+++ b/tests/qapi-schema/alias-source-optional-wildcard.json
@@ -0,0 +1,5 @@
+{ 'struct': 'Nested',
+ 'data': { 'foo': 'int' } }
+{ 'struct': 'AliasStruct0',
+ 'data': { '*nested': 'Nested' },
+ 'aliases': [ { 'source': ['nested'] } ] }
diff --git a/tests/qapi-schema/alias-source-optional-wildcard.out b/tests/qapi-schema/alias-source-optional-wildcard.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-unknown-key.err b/tests/qapi-schema/alias-unknown-key.err
new file mode 100644
index 0000000000..c7b8cb9498
--- /dev/null
+++ b/tests/qapi-schema/alias-unknown-key.err
@@ -0,0 +1,3 @@
+alias-unknown-key.json: In struct 'AliasStruct0':
+alias-unknown-key.json:1: 'aliases' member has unknown key 'known'
+Valid keys are 'name', 'source'.
diff --git a/tests/qapi-schema/alias-unknown-key.json b/tests/qapi-schema/alias-unknown-key.json
new file mode 100644
index 0000000000..cdb8fc3d07
--- /dev/null
+++ b/tests/qapi-schema/alias-unknown-key.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': ['foo'], 'known': false } ] }
diff --git a/tests/qapi-schema/alias-unknown-key.out b/tests/qapi-schema/alias-unknown-key.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/aliases-bad-type.err b/tests/qapi-schema/aliases-bad-type.err
new file mode 100644
index 0000000000..7ffe789ec0
--- /dev/null
+++ b/tests/qapi-schema/aliases-bad-type.err
@@ -0,0 +1,2 @@
+aliases-bad-type.json: In struct 'AliasStruct0':
+aliases-bad-type.json:1: 'aliases' must be an array
diff --git a/tests/qapi-schema/aliases-bad-type.json b/tests/qapi-schema/aliases-bad-type.json
new file mode 100644
index 0000000000..4bbf6d6b20
--- /dev/null
+++ b/tests/qapi-schema/aliases-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': 'this must be an array' }
diff --git a/tests/qapi-schema/aliases-bad-type.out b/tests/qapi-schema/aliases-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index b8de58116a..f937de1c35 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -3,6 +3,22 @@ test_env.set('PYTHONPATH', meson.source_root() / 'scripts')
test_env.set('PYTHONIOENCODING', 'utf-8')
schemas = [
+ 'alias-bad-type.json',
+ 'aliases-bad-type.json',
+ 'alias-missing-source.json',
+ 'alias-name-bad-type.json',
+ 'alias-name-conflict.json',
+ 'alias-recursive.json',
+ 'alias-source-bad-type.json',
+ 'alias-source-elem-bad-type.json',
+ 'alias-source-empty.json',
+ 'alias-source-inexistent.json',
+ 'alias-source-inexistent-variants.json',
+ 'alias-source-non-object-path.json',
+ 'alias-source-non-object-wildcard.json',
+ 'alias-source-optional-wildcard.json',
+ 'alias-source-optional-wildcard-indirect.json',
+ 'alias-unknown-key.json',
'alternate-any.json',
'alternate-array.json',
'alternate-base.json',
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 84b9d41f15..c5e81a883c 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -336,3 +336,29 @@
{ 'event': 'TEST_EVENT_FEATURES1',
'features': [ 'deprecated' ] }
+
+# test 'aliases'
+
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [] }
+{ 'struct': 'AliasStruct1',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': ['foo'] } ] }
+{ 'struct': 'AliasStruct2',
+ 'data': { 'nested': 'AliasStruct1' },
+ 'aliases': [ { 'name': 'bar', 'source': ['nested', 'foo'] } ] }
+{ 'struct': 'AliasStruct3',
+ 'data': { 'nested': 'AliasStruct1' },
+ 'aliases': [ { 'source': ['nested'] } ] }
+
+{ 'union': 'AliasFlatUnion',
+ 'base': { 'tag': 'FeatureEnum1' },
+ 'discriminator': 'tag',
+ 'data': { 'eins': 'FeatureStruct1' },
+ 'aliases': [ { 'name': 'variant', 'source': ['tag'] },
+ { 'name': 'bar', 'source': ['foo'] } ] }
+{ 'union': 'AliasSimpleUnion',
+ 'data': { 'eins': 'AliasStruct1' },
+ 'aliases': [ { 'source': ['data'] },
+ { 'name': 'tag', 'source': ['type'] } ] }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index e0b8a5f0b6..f6b8a98b7c 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -445,6 +445,37 @@ event TEST_EVENT_FEATURES0 FeatureStruct1
event TEST_EVENT_FEATURES1 None
boxed=False
feature deprecated
+object AliasStruct0
+ member foo: int optional=False
+object AliasStruct1
+ member foo: int optional=False
+ alias bar -> foo
+object AliasStruct2
+ member nested: AliasStruct1 optional=False
+ alias bar -> nested.foo
+object AliasStruct3
+ member nested: AliasStruct1 optional=False
+ alias * -> nested.*
+object q_obj_AliasFlatUnion-base
+ member tag: FeatureEnum1 optional=False
+object AliasFlatUnion
+ base q_obj_AliasFlatUnion-base
+ alias variant -> tag
+ alias bar -> foo
+ tag tag
+ case eins: FeatureStruct1
+ case zwei: q_empty
+ case drei: q_empty
+object q_obj_AliasStruct1-wrapper
+ member data: AliasStruct1 optional=False
+enum AliasSimpleUnionKind
+ member eins
+object AliasSimpleUnion
+ member type: AliasSimpleUnionKind optional=False
+ alias * -> data.*
+ alias tag -> type
+ tag type
+ case eins: q_obj_AliasStruct1-wrapper
module include/sub-module.json
include sub-sub-module.json
object SecondArrayRef
--
2.31.1
Kevin Wolf <kwolf@redhat.com> writes:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
[...]
> diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
> index e41b91a2a6..f2891b6f5d 100644
> --- a/tests/unit/test-qobject-input-visitor.c
> +++ b/tests/unit/test-qobject-input-visitor.c
> @@ -952,6 +952,214 @@ static void test_visitor_in_list_union_number(TestInputVisitorData *data,
> g_string_free(gstr_list, true);
> }
>
> +static void test_visitor_in_alias_struct_local(TestInputVisitorData *data,
> + const void *unused)
> +{
> + AliasStruct1 *tmp = NULL;
> + Error *err = NULL;
> + Visitor *v;
> +
Context: the schema makes 'bar' an alias for 'foo'.
> + /* Can still specify the real member name with alias support */
> + v = visitor_input_test_init(data, "{ 'foo': 42 }");
> + visit_type_AliasStruct1(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->foo, ==, 42);
> + qapi_free_AliasStruct1(tmp);
> +
> + /* The alias is a working alternative */
> + v = visitor_input_test_init(data, "{ 'bar': 42 }");
> + visit_type_AliasStruct1(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->foo, ==, 42);
> + qapi_free_AliasStruct1(tmp);
> +
> + /* But you can't use both at the same time */
> + v = visitor_input_test_init(data, "{ 'foo': 5, 'bar': 42 }");
> + visit_type_AliasStruct1(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
I double-checked this reports "Value for parameter foo was already given
through an alias", as it should.
Pointing to what exactly is giving values to foo already would be nice.
In this case, 'foo' is obvious, but 'bar' is not. This is not a demand.
> +}
> +
> +static void test_visitor_in_alias_struct_nested(TestInputVisitorData *data,
> + const void *unused)
> +{
> + AliasStruct2 *tmp = NULL;
> + Error *err = NULL;
> + Visitor *v;
> +
Context: the schema makes 'bar' and 'nested.bar' aliases for
'nested.foo'.
> + /* Can still specify the real member names with alias support */
> + v = visitor_input_test_init(data, "{ 'nested': { 'foo': 42 } }");
> + visit_type_AliasStruct2(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->nested->foo, ==, 42);
> + qapi_free_AliasStruct2(tmp);
> +
> + /* The inner alias is a working alternative */
> + v = visitor_input_test_init(data, "{ 'nested': { 'bar': 42 } }");
> + visit_type_AliasStruct2(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->nested->foo, ==, 42);
> + qapi_free_AliasStruct2(tmp);
> +
> + /* So is the outer alias */
> + v = visitor_input_test_init(data, "{ 'bar': 42 }");
> + visit_type_AliasStruct2(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->nested->foo, ==, 42);
> + qapi_free_AliasStruct2(tmp);
> +
> + /* You can't use more than one option at the same time */
> + v = visitor_input_test_init(data, "{ 'bar': 5, 'nested': { 'foo': 42 } }");
> + visit_type_AliasStruct2(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
"Value for parameter nested.foo was already given through an alias".
Good.
> +
> + v = visitor_input_test_init(data, "{ 'bar': 5, 'nested': { 'bar': 42 } }");
> + visit_type_AliasStruct2(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
Likewise.
> +
> + v = visitor_input_test_init(data, "{ 'nested': { 'foo': 42, 'bar': 42 } }");
> + visit_type_AliasStruct2(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
Likewise.
> +
> + v = visitor_input_test_init(data, "{ 'bar': 5, "
> + " 'nested': { 'foo': 42, 'bar': 42 } }");
> + visit_type_AliasStruct2(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
Likewise.
In the second of these four cases, none of the things giving values to
nested.foo is obvious. Still not a demand.
> +}
> +
> +static void test_visitor_in_alias_wildcard(TestInputVisitorData *data,
> + const void *unused)
> +{
> + AliasStruct3 *tmp = NULL;
> + Error *err = NULL;
> + Visitor *v;
> +
Context: the schema makes 'foo', 'bar', and 'nested.bar' aliases for
'nested.foo', using a wildcard alias for the former two.
> + /* Can still specify the real member names with alias support */
> + v = visitor_input_test_init(data, "{ 'nested': { 'foo': 42 } }");
> + visit_type_AliasStruct3(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->nested->foo, ==, 42);
> + qapi_free_AliasStruct3(tmp);
> +
> + /* The wildcard alias makes it work on the top level */
> + v = visitor_input_test_init(data, "{ 'foo': 42 }");
> + visit_type_AliasStruct3(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->nested->foo, ==, 42);
> + qapi_free_AliasStruct3(tmp);
> +
> + /* It makes the inner alias available, too */
> + v = visitor_input_test_init(data, "{ 'bar': 42 }");
> + visit_type_AliasStruct3(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->nested->foo, ==, 42);
> + qapi_free_AliasStruct3(tmp);
> +
> + /* You can't use more than one option at the same time */
> + v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'foo': 42 } }");
> + visit_type_AliasStruct3(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
"Parameter 'foo' is unexpected". Say what? It *is* expected, it just
clashes with 'nested.foo'.
I figure this is what happens:
* visit_type_AliasStruct3()
- visit_start_struct()
- visit_type_AliasStruct3_members()
• visit_type_AliasStruct1() for member @nested.
This consumes consumes input nested.foo.
- visit_check_struct()
Error: input foo has not been consumed.
Any ideas on how to report this error more clearly?
> +
> + v = visitor_input_test_init(data, "{ 'bar': 42, 'nested': { 'foo': 42 } }");
> + visit_type_AliasStruct3(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
"Value for parameter nested.foo was already given through an alias".
Good (but I have no idea how we avoid the bad error reporting in this
case).
> +
> + v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'bar': 42 } }");
> + visit_type_AliasStruct3(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
"Parameter 'foo' is unexpected"
> +
> + v = visitor_input_test_init(data, "{ 'bar': 42, 'nested': { 'bar': 42 } }");
> + visit_type_AliasStruct3(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
"Parameter 'bar' is unexpected"
> +
> + v = visitor_input_test_init(data, "{ 'foo': 42, 'bar': 42 }");
> + visit_type_AliasStruct3(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
"Parameter 'foo' is unexpected"
> +}
> +
> +static void test_visitor_in_alias_flat_union(TestInputVisitorData *data,
> + const void *unused)
> +{
> + AliasFlatUnion *tmp = NULL;
> + Error *err = NULL;
> + Visitor *v;
> +
Context: the schema makes 'variant' an alias for 'tag', and 'bar' an
alias for 'foo'.
> + /* Can still specify the real member name with alias support */
> + v = visitor_input_test_init(data, "{ 'tag': 'drei' }");
> + visit_type_AliasFlatUnion(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->tag, ==, FEATURE_ENUM1_DREI);
> + qapi_free_AliasFlatUnion(tmp);
> +
> + /* Use alias for a base member (the discriminator even) */
> + v = visitor_input_test_init(data, "{ 'variant': 'zwei' }");
> + visit_type_AliasFlatUnion(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->tag, ==, FEATURE_ENUM1_ZWEI);
> + qapi_free_AliasFlatUnion(tmp);
> +
> + /* Use alias for a variant member */
> + v = visitor_input_test_init(data, "{ 'tag': 'eins', 'bar': 42 }");
> + visit_type_AliasFlatUnion(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->tag, ==, FEATURE_ENUM1_EINS);
> + g_assert_cmpint(tmp->u.eins.foo, ==, 42);
> + qapi_free_AliasFlatUnion(tmp);
> +
> + /* Both together */
> + v = visitor_input_test_init(data, "{ 'variant': 'eins', 'bar': 42 }");
> + visit_type_AliasFlatUnion(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->tag, ==, FEATURE_ENUM1_EINS);
> + g_assert_cmpint(tmp->u.eins.foo, ==, 42);
> + qapi_free_AliasFlatUnion(tmp);
> +
> + /* You can't use more than one option at the same time for each alias */
> + v = visitor_input_test_init(data, "{ 'variant': 'zwei', 'tag': 'drei' }");
> + visit_type_AliasFlatUnion(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
"Value for parameter tag was already given through an alias". Good.
> +
> + v = visitor_input_test_init(data, "{ 'tag': 'eins', 'foo': 6, 'bar': 9 }");
> + visit_type_AliasFlatUnion(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
"Value for parameter foo was already given through an alias". Good,
except I'm getting a feeling "already" may be confusing. It's "already"
only in the sense that we already got the value via alias, which is an
implementation detail. It may or may not be given already in the
input. Here it's not: 'bar' follows 'foo'.
What about "is also given through an alias"?
> +}
> +
> +static void test_visitor_in_alias_simple_union(TestInputVisitorData *data,
> + const void *unused)
> +{
> + AliasSimpleUnion *tmp = NULL;
> + Error *err = NULL;
> + Visitor *v;
> +
Context: the schema makes 'foo' and 'bar' aliases for 'data.foo' and
'data.bar' (using wildcard alias), 'tag' an alias for 'type', and
'data.bar' an alias for 'data.foo'.
> + /* Can still specify the real member name with alias support */
> + v = visitor_input_test_init(data, "{ 'type': 'eins', "
> + " 'data': { 'foo': 42 } }");
> + visit_type_AliasSimpleUnion(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->type, ==, ALIAS_SIMPLE_UNION_KIND_EINS);
> + g_assert_cmpint(tmp->u.eins.data->foo, ==, 42);
> + qapi_free_AliasSimpleUnion(tmp);
> +
> + /* 'type' can be aliased */
> + v = visitor_input_test_init(data, "{ 'tag': 'eins', "
> + " 'data': { 'foo': 42 } }");
> + visit_type_AliasSimpleUnion(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->type, ==, ALIAS_SIMPLE_UNION_KIND_EINS);
> + g_assert_cmpint(tmp->u.eins.data->foo, ==, 42);
> + qapi_free_AliasSimpleUnion(tmp);
> +
> + /* The wildcard alias makes it work on the top level */
> + v = visitor_input_test_init(data, "{ 'type': 'eins', 'foo': 42 }");
> + visit_type_AliasSimpleUnion(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->type, ==, ALIAS_SIMPLE_UNION_KIND_EINS);
> + g_assert_cmpint(tmp->u.eins.data->foo, ==, 42);
> + qapi_free_AliasSimpleUnion(tmp);
> +
> + /* It makes the inner alias available, too */
> + v = visitor_input_test_init(data, "{ 'type': 'eins', 'bar': 42 }");
> + visit_type_AliasSimpleUnion(v, NULL, &tmp, &error_abort);
> + g_assert_cmpint(tmp->type, ==, ALIAS_SIMPLE_UNION_KIND_EINS);
> + g_assert_cmpint(tmp->u.eins.data->foo, ==, 42);
> + qapi_free_AliasSimpleUnion(tmp);
> +
> + /* You can't use more than one option at the same time for each alias */
> + v = visitor_input_test_init(data, "{ 'type': 'eins', 'tag': 'eins' }");
> + visit_type_AliasSimpleUnion(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
"Value for parameter type was already given through an alias". Good,
except "parameter type" is confusing. Make it "parameter 'type'".
> +
> + v = visitor_input_test_init(data, "{ 'type': 'eins', "
> + " 'bar': 123, "
> + " 'data': { 'foo': 312 } }");
> + visit_type_AliasSimpleUnion(v, NULL, &tmp, &err);
> + error_free_or_abort(&err);
"Value for parameter data.foo was already given through an alias".
Good.
> +}
> +
> static void input_visitor_test_add(const char *testpath,
> const void *user_data,
> void (*test_func)(TestInputVisitorData *data,
> @@ -1350,6 +1558,16 @@ int main(int argc, char **argv)
> NULL, test_visitor_in_list_union_string);
> input_visitor_test_add("/visitor/input/list_union/number",
> NULL, test_visitor_in_list_union_number);
> + input_visitor_test_add("/visitor/input/alias/struct-local",
> + NULL, test_visitor_in_alias_struct_local);
> + input_visitor_test_add("/visitor/input/alias/struct-nested",
> + NULL, test_visitor_in_alias_struct_nested);
> + input_visitor_test_add("/visitor/input/alias/wildcard",
> + NULL, test_visitor_in_alias_wildcard);
> + input_visitor_test_add("/visitor/input/alias/flat-union",
> + NULL, test_visitor_in_alias_flat_union);
> + input_visitor_test_add("/visitor/input/alias/simple-union",
> + NULL, test_visitor_in_alias_simple_union);
> input_visitor_test_add("/visitor/input/fail/struct",
> NULL, test_visitor_in_fail_struct);
> input_visitor_test_add("/visitor/input/fail/struct-nested",
[Negative tests snipped, I checked them in review of PATCH 5, they're
fine]
> diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
> index b8de58116a..f937de1c35 100644
> --- a/tests/qapi-schema/meson.build
> +++ b/tests/qapi-schema/meson.build
> @@ -3,6 +3,22 @@ test_env.set('PYTHONPATH', meson.source_root() / 'scripts')
> test_env.set('PYTHONIOENCODING', 'utf-8')
>
> schemas = [
> + 'alias-bad-type.json',
> + 'aliases-bad-type.json',
> + 'alias-missing-source.json',
> + 'alias-name-bad-type.json',
> + 'alias-name-conflict.json',
> + 'alias-recursive.json',
> + 'alias-source-bad-type.json',
> + 'alias-source-elem-bad-type.json',
> + 'alias-source-empty.json',
> + 'alias-source-inexistent.json',
> + 'alias-source-inexistent-variants.json',
> + 'alias-source-non-object-path.json',
> + 'alias-source-non-object-wildcard.json',
> + 'alias-source-optional-wildcard.json',
> + 'alias-source-optional-wildcard-indirect.json',
> + 'alias-unknown-key.json',
> 'alternate-any.json',
> 'alternate-array.json',
> 'alternate-base.json',
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 84b9d41f15..c5e81a883c 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -336,3 +336,29 @@
>
> { 'event': 'TEST_EVENT_FEATURES1',
> 'features': [ 'deprecated' ] }
> +
> +# test 'aliases'
> +
> +{ 'struct': 'AliasStruct0',
> + 'data': { 'foo': 'int' },
> + 'aliases': [] }
> +{ 'struct': 'AliasStruct1',
> + 'data': { 'foo': 'int' },
> + 'aliases': [ { 'name': 'bar', 'source': ['foo'] } ] }
> +{ 'struct': 'AliasStruct2',
> + 'data': { 'nested': 'AliasStruct1' },
> + 'aliases': [ { 'name': 'bar', 'source': ['nested', 'foo'] } ] }
> +{ 'struct': 'AliasStruct3',
> + 'data': { 'nested': 'AliasStruct1' },
> + 'aliases': [ { 'source': ['nested'] } ] }
> +
> +{ 'union': 'AliasFlatUnion',
> + 'base': { 'tag': 'FeatureEnum1' },
> + 'discriminator': 'tag',
> + 'data': { 'eins': 'FeatureStruct1' },
> + 'aliases': [ { 'name': 'variant', 'source': ['tag'] },
> + { 'name': 'bar', 'source': ['foo'] } ] }
> +{ 'union': 'AliasSimpleUnion',
> + 'data': { 'eins': 'AliasStruct1' },
> + 'aliases': [ { 'source': ['data'] },
> + { 'name': 'tag', 'source': ['type'] } ] }
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index e0b8a5f0b6..f6b8a98b7c 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -445,6 +445,37 @@ event TEST_EVENT_FEATURES0 FeatureStruct1
> event TEST_EVENT_FEATURES1 None
> boxed=False
> feature deprecated
> +object AliasStruct0
> + member foo: int optional=False
> +object AliasStruct1
> + member foo: int optional=False
> + alias bar -> foo
> +object AliasStruct2
> + member nested: AliasStruct1 optional=False
> + alias bar -> nested.foo
> +object AliasStruct3
> + member nested: AliasStruct1 optional=False
> + alias * -> nested.*
> +object q_obj_AliasFlatUnion-base
> + member tag: FeatureEnum1 optional=False
> +object AliasFlatUnion
> + base q_obj_AliasFlatUnion-base
> + alias variant -> tag
> + alias bar -> foo
> + tag tag
> + case eins: FeatureStruct1
> + case zwei: q_empty
> + case drei: q_empty
> +object q_obj_AliasStruct1-wrapper
> + member data: AliasStruct1 optional=False
> +enum AliasSimpleUnionKind
> + member eins
> +object AliasSimpleUnion
> + member type: AliasSimpleUnionKind optional=False
> + alias * -> data.*
> + alias tag -> type
> + tag type
> + case eins: q_obj_AliasStruct1-wrapper
> module include/sub-module.json
> include sub-sub-module.json
> object SecondArrayRef
Positive tests look good to me, except they neglect to use any of the
types using the alias features in QMP. I think we need something like
the appended incremental patch.
Oh, with that, backing out the hunk
- members = seen.values()
+ members = list(seen.values())
as described in review of PATCH 5 actually fails "make check"!
The generated test-qapi-introspect.c doesn't show aliases. Here's
AliasStruct1:
/* "63" = AliasStruct1 */
QLIT_QDICT(((QLitDictEntry[]) {
{ "members", QLIT_QLIST(((QLitObject[]) {
QLIT_QDICT(((QLitDictEntry[]) {
{ "name", QLIT_QSTR("foo"), },
{ "type", QLIT_QSTR("int"), },
{}
})),
{}
})), },
{ "meta-type", QLIT_QSTR("object"), },
{ "name", QLIT_QSTR("63"), },
{}
})),
Not a peep about member 'bar'.
We need to address this for use case "compatible schema evolution", so
that management applications can detect presence of the new interface.
Actual use of aliases for this purpose requires coordination with
libvirt developers, of course.
How could introspection show aliases? We can't simply add an entry for
"bar" to "members", because that would show two mandatory members "foo"
and "bar", which is wrong.
If we add "aliases" next to "members", aliases remain invisible for
older management applications. I don't have better ideas.
Let's have a closer look at "compatible schema evolution". We want to
move / rename a member, and use aliases to support both the new and the
old name for compatibility. We want to be able to deprecate the old
name.
Example 1: move 'foo' to 'bar'
Two ways:
1. Replace member 'foo' by 'bar', then add alias 'foo'
Old management applications can't see the alias. To them, it
looks like 'foo' vanished without replacement, which is a
compatibility break. May well cause trouble.
2. Add alias 'bar'
Old management applications can't see the alias. If we deprecate
'foo', they see that. Unlikely to cause trouble, I think. If we
remove 'foo', compatibility break, but that's intentional.
Always use way 2. Documentation should spell that out.
Example 2: move 'nested.foo' to 'bar'
Due to the way aliases work, we need to make 'bar' the alias, like
'aliases': [ { 'name': 'bar', 'source': ['nested', 'foo'] } ] }
This is way 2. again. Fine.
Example 1: move 'bar' to 'nested.foo'
Due to the way aliases work, we need to replace 'bar' by
'nested.foo', then add alias 'bar'.
Here, we can only use problematic way 1. Better ideas than
"document the limitation?"
diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
index 1b0b7d99df..907468b157 100644
--- a/tests/unit/test-qmp-cmds.c
+++ b/tests/unit/test-qmp-cmds.c
@@ -76,6 +76,16 @@ void qmp_test_command_cond_features3(Error **errp)
{
}
+void qmp_test_aliases0(bool has_as0, AliasStruct0 *as0,
+ bool has_as1, AliasStruct1 *as1,
+ bool has_as2, AliasStruct2 *as2,
+ bool has_as3, AliasStruct3 *as3,
+ bool has_afu, AliasFlatUnion *afu,
+ bool has_asu, AliasSimpleUnion *asu,
+ Error **errp)
+{
+}
+
UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
bool has_udb1, UserDefOne *ud1b,
Error **errp)
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index c5e81a883c..4d3a5039b4 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -362,3 +362,11 @@
'data': { 'eins': 'AliasStruct1' },
'aliases': [ { 'source': ['data'] },
{ 'name': 'tag', 'source': ['type'] } ] }
+
+{ 'command': 'test-aliases0',
+ 'data': { '*as0': 'AliasStruct0',
+ '*as1': 'AliasStruct1',
+ '*as2': 'AliasStruct2',
+ '*as3': 'AliasStruct3',
+ '*afu': 'AliasFlatUnion',
+ '*asu': 'AliasSimpleUnion' } }
Am 06.09.2021 um 17:28 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> > + /* Can still specify the real member name with alias support */
> > + v = visitor_input_test_init(data, "{ 'foo': 42 }");
> > + visit_type_AliasStruct1(v, NULL, &tmp, &error_abort);
> > + g_assert_cmpint(tmp->foo, ==, 42);
> > + qapi_free_AliasStruct1(tmp);
> > +
> > + /* The alias is a working alternative */
> > + v = visitor_input_test_init(data, "{ 'bar': 42 }");
> > + visit_type_AliasStruct1(v, NULL, &tmp, &error_abort);
> > + g_assert_cmpint(tmp->foo, ==, 42);
> > + qapi_free_AliasStruct1(tmp);
> > +
> > + /* But you can't use both at the same time */
> > + v = visitor_input_test_init(data, "{ 'foo': 5, 'bar': 42 }");
> > + visit_type_AliasStruct1(v, NULL, &tmp, &err);
> > + error_free_or_abort(&err);
>
> I double-checked this reports "Value for parameter foo was already given
> through an alias", as it should.
>
> Pointing to what exactly is giving values to foo already would be nice.
> In this case, 'foo' is obvious, but 'bar' is not. This is not a demand.
We have the name, so we could print it, but it could be in a different
StackObject. I'm not sure if we have a good way to identify a parent
StackObject, and without it the message could be very confusing.
If you have a good idea what the message should look like, I can make an
attempt.
> > + /* You can't use more than one option at the same time */
> > + v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'foo': 42 } }");
> > + visit_type_AliasStruct3(v, NULL, &tmp, &err);
> > + error_free_or_abort(&err);
>
> "Parameter 'foo' is unexpected". Say what? It *is* expected, it just
> clashes with 'nested.foo'.
>
> I figure this is what happens:
>
> * visit_type_AliasStruct3()
>
> - visit_start_struct()
>
> - visit_type_AliasStruct3_members()
>
> • visit_type_AliasStruct1() for member @nested.
>
> This consumes consumes input nested.foo.
>
> - visit_check_struct()
>
> Error: input foo has not been consumed.
>
> Any ideas on how to report this error more clearly?
It's a result of the logic that wildcard aliases are silently ignored
when they aren't needed. The reason why I included this is that it would
allow you to have two members with the same name in the object
containing the alias and in the aliased object without conflicting as
long as both are given.
Never skipping wildcard aliases makes the code simpler and results in
the expected error message here. So I'll do that for v4.
Note that parsing something like '--chardev type=socket,addr.type=unix,
path=/tmp/sock,id=c' now depends on the order in the generated code. If
the top level 'type' weren't parsed and removed from the input first,
visiting 'addr.type' would now detect a conflict. For union types, we
know that 'type' is always parsed first, so it's not a problem, but in
the general case you need to be careful with the order.
> > +
> > + v = visitor_input_test_init(data, "{ 'tag': 'eins', 'foo': 6, 'bar': 9 }");
> > + visit_type_AliasFlatUnion(v, NULL, &tmp, &err);
> > + error_free_or_abort(&err);
>
> "Value for parameter foo was already given through an alias". Good,
> except I'm getting a feeling "already" may be confusing. It's "already"
> only in the sense that we already got the value via alias, which is an
> implementation detail. It may or may not be given already in the
> input. Here it's not: 'bar' follows 'foo'.
>
> What about "is also given through an alias"?
Sounds good.
> Positive tests look good to me, except they neglect to use any of the
> types using the alias features in QMP. I think we need something like
> the appended incremental patch.
I'm squashing it in.
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 06.09.2021 um 17:28 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > + /* Can still specify the real member name with alias support */
>> > + v = visitor_input_test_init(data, "{ 'foo': 42 }");
>> > + visit_type_AliasStruct1(v, NULL, &tmp, &error_abort);
>> > + g_assert_cmpint(tmp->foo, ==, 42);
>> > + qapi_free_AliasStruct1(tmp);
>> > +
>> > + /* The alias is a working alternative */
>> > + v = visitor_input_test_init(data, "{ 'bar': 42 }");
>> > + visit_type_AliasStruct1(v, NULL, &tmp, &error_abort);
>> > + g_assert_cmpint(tmp->foo, ==, 42);
>> > + qapi_free_AliasStruct1(tmp);
>> > +
>> > + /* But you can't use both at the same time */
>> > + v = visitor_input_test_init(data, "{ 'foo': 5, 'bar': 42 }");
>> > + visit_type_AliasStruct1(v, NULL, &tmp, &err);
>> > + error_free_or_abort(&err);
>>
>> I double-checked this reports "Value for parameter foo was already given
>> through an alias", as it should.
>>
>> Pointing to what exactly is giving values to foo already would be nice.
>> In this case, 'foo' is obvious, but 'bar' is not. This is not a demand.
>
> We have the name, so we could print it, but it could be in a different
> StackObject. I'm not sure if we have a good way to identify a parent
> StackObject, and without it the message could be very confusing.
All we have is full_name_so(), which can describe members / aliases in
any StackObject currently on qiv->stack, i.e. in the current object, the
object that contains it, and so forth up to the root object.
> If you have a good idea what the message should look like, I can make an
> attempt.
Of course, users don't want to know about alias chains, they want to
know what part of their input makes things explode. The answer to that
question could be like "'a.b.c' clashes with 'c'" (too terse, but you
get the idea).
Perhaps users then want to know *why* it clashes. To answer that
question, we'd have to present the complete alias chain. I'm not sure
this is worth the trouble.
>> > + /* You can't use more than one option at the same time */
>> > + v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'foo': 42 } }");
>> > + visit_type_AliasStruct3(v, NULL, &tmp, &err);
>> > + error_free_or_abort(&err);
>>
>> "Parameter 'foo' is unexpected". Say what? It *is* expected, it just
>> clashes with 'nested.foo'.
>>
>> I figure this is what happens:
>>
>> * visit_type_AliasStruct3()
>>
>> - visit_start_struct()
>>
>> - visit_type_AliasStruct3_members()
>>
>> • visit_type_AliasStruct1() for member @nested.
>>
>> This consumes consumes input nested.foo.
>>
>> - visit_check_struct()
>>
>> Error: input foo has not been consumed.
>>
>> Any ideas on how to report this error more clearly?
>
> It's a result of the logic that wildcard aliases are silently ignored
> when they aren't needed. The reason why I included this is that it would
> allow you to have two members with the same name in the object
> containing the alias and in the aliased object without conflicting as
> long as both are given.
*brain cramp*
Example?
> Never skipping wildcard aliases makes the code simpler and results in
> the expected error message here. So I'll do that for v4.
Trusting your judgement.
> Note that parsing something like '--chardev type=socket,addr.type=unix,
> path=/tmp/sock,id=c' now depends on the order in the generated code. If
> the top level 'type' weren't parsed and removed from the input first,
> visiting 'addr.type' would now detect a conflict. For union types, we
> know that 'type' is always parsed first, so it's not a problem, but in
> the general case you need to be careful with the order.
Uff! I think I'd like to understand this better. No need to delay v4
for that.
Can't yet say whether we need to spell out the limitation in commit
message and/or documentation.
>> > +
>> > + v = visitor_input_test_init(data, "{ 'tag': 'eins', 'foo': 6, 'bar': 9 }");
>> > + visit_type_AliasFlatUnion(v, NULL, &tmp, &err);
>> > + error_free_or_abort(&err);
>>
>> "Value for parameter foo was already given through an alias". Good,
>> except I'm getting a feeling "already" may be confusing. It's "already"
>> only in the sense that we already got the value via alias, which is an
>> implementation detail. It may or may not be given already in the
>> input. Here it's not: 'bar' follows 'foo'.
>>
>> What about "is also given through an alias"?
>
> Sounds good.
>
>> Positive tests look good to me, except they neglect to use any of the
>> types using the alias features in QMP. I think we need something like
>> the appended incremental patch.
>
> I'm squashing it in.
Thanks!
Am 14.09.2021 um 10:59 hat Markus Armbruster geschrieben:
> >> > + /* You can't use more than one option at the same time */
> >> > + v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'foo': 42 } }");
> >> > + visit_type_AliasStruct3(v, NULL, &tmp, &err);
> >> > + error_free_or_abort(&err);
> >>
> >> "Parameter 'foo' is unexpected". Say what? It *is* expected, it just
> >> clashes with 'nested.foo'.
> >>
> >> I figure this is what happens:
> >>
> >> * visit_type_AliasStruct3()
> >>
> >> - visit_start_struct()
> >>
> >> - visit_type_AliasStruct3_members()
> >>
> >> • visit_type_AliasStruct1() for member @nested.
> >>
> >> This consumes consumes input nested.foo.
> >>
> >> - visit_check_struct()
> >>
> >> Error: input foo has not been consumed.
> >>
> >> Any ideas on how to report this error more clearly?
> >
> > It's a result of the logic that wildcard aliases are silently ignored
> > when they aren't needed. The reason why I included this is that it would
> > allow you to have two members with the same name in the object
> > containing the alias and in the aliased object without conflicting as
> > long as both are given.
>
> *brain cramp*
>
> Example?
Let's use the real-world example I mentioned below:
{ 'union': 'ChardevBackend',
'data': { ...,
'socket': 'ChardevSocket',
... },
'aliases': [ { 'source': ['data'] } ] }
{ 'struct': 'ChardevSocket',
'data': { 'addr': 'SocketAddressLegacy',
... },
'base': 'ChardevCommon',
'aliases': [ { 'source': ['addr'] } ] }
{ 'union': 'SocketAddressLegacy',
'data': {
'inet': 'InetSocketAddress',
'unix': 'UnixSocketAddress',
'vsock': 'VsockSocketAddress',
'fd': 'String' },
'aliases': [
{ 'source': ['data'] },
{ 'name': 'fd', 'source': ['data', 'str'] }
] }
We have two simple unions there, and wildcard aliases all the way
through, so that you can have things like "hostname" on the top level.
However, two simple unions mean that "type" could refer to either
ChardevBackend.type or to SocketAddressLegacy.type.
Even though strictly speaking 'type=socket' is ambiguous, you don't want
to error out, but interpret it as a value for the outer one.
> > Never skipping wildcard aliases makes the code simpler and results in
> > the expected error message here. So I'll do that for v4.
>
> Trusting your judgement.
>
> > Note that parsing something like '--chardev type=socket,addr.type=unix,
> > path=/tmp/sock,id=c' now depends on the order in the generated code. If
> > the top level 'type' weren't parsed and removed from the input first,
> > visiting 'addr.type' would now detect a conflict. For union types, we
> > know that 'type' is always parsed first, so it's not a problem, but in
> > the general case you need to be careful with the order.
>
> Uff! I think I'd like to understand this better. No need to delay v4
> for that.
>
> Can't yet say whether we need to spell out the limitation in commit
> message and/or documentation.
The point where we could error out is when parsing SocketAddressLegacy,
because there can be two possible providers for "type".
The idea in the current code of this series was that we'll just ignore
wildcard aliases if we already have a value, because then the value must
be meant for somewhere else. So it doesn't error out and leaves the
value in the QDict for someone else to pick it up. If nobody picks it
up, it's an error "'foo' is unexpected".
If we change it and do error out when there are multiple possible values
through wildcard aliases, then the only thing that makes it work is that
ChardevBackend.type is parsed first and is therefore not a possible
value for SocketAddressLegacy.type any more.
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 14.09.2021 um 10:59 hat Markus Armbruster geschrieben:
>> >> > + /* You can't use more than one option at the same time */
>> >> > + v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'foo': 42 } }");
>> >> > + visit_type_AliasStruct3(v, NULL, &tmp, &err);
>> >> > + error_free_or_abort(&err);
>> >>
>> >> "Parameter 'foo' is unexpected". Say what? It *is* expected, it just
>> >> clashes with 'nested.foo'.
>> >>
>> >> I figure this is what happens:
>> >>
>> >> * visit_type_AliasStruct3()
>> >>
>> >> - visit_start_struct()
>> >>
>> >> - visit_type_AliasStruct3_members()
>> >>
>> >> • visit_type_AliasStruct1() for member @nested.
>> >>
>> >> This consumes consumes input nested.foo.
>> >>
>> >> - visit_check_struct()
>> >>
>> >> Error: input foo has not been consumed.
>> >>
>> >> Any ideas on how to report this error more clearly?
>> >
>> > It's a result of the logic that wildcard aliases are silently ignored
>> > when they aren't needed. The reason why I included this is that it would
>> > allow you to have two members with the same name in the object
>> > containing the alias and in the aliased object without conflicting as
>> > long as both are given.
>>
>> *brain cramp*
>>
>> Example?
>
> Let's use the real-world example I mentioned below:
>
> { 'union': 'ChardevBackend',
> 'data': { ...,
> 'socket': 'ChardevSocket',
> ... },
> 'aliases': [ { 'source': ['data'] } ] }
To pretend the simple union was flat, i.e. peel off its 'data', because
that nesting doesn't exist in the CLI you want to QAPIfy.
>
> { 'struct': 'ChardevSocket',
> 'data': { 'addr': 'SocketAddressLegacy',
> ... },
> 'base': 'ChardevCommon',
> 'aliases': [ { 'source': ['addr'] } ] }
To unbox struct SocketAddressLegacy, i.e. peel off its 'addr', for the
same reason.
>
> { 'union': 'SocketAddressLegacy',
> 'data': {
> 'inet': 'InetSocketAddress',
> 'unix': 'UnixSocketAddress',
> 'vsock': 'VsockSocketAddress',
> 'fd': 'String' },
> 'aliases': [
> { 'source': ['data'] },
To pretend the simple union was flat, i.e. peel off its 'data',
> { 'name': 'fd', 'source': ['data', 'str'] }
To unbox struct String, i.e. peel off its 'data'.
> ] }
Okay, I understand what you're trying to do. However:
> We have two simple unions there, and wildcard aliases all the way
> through, so that you can have things like "hostname" on the top level.
> However, two simple unions mean that "type" could refer to either
> ChardevBackend.type or to SocketAddressLegacy.type.
Yup. In ChardevBackend, we have both a (common) member @type, and a
chain of aliases that resolves @type to data.addr.data.type.
> Even though strictly speaking 'type=socket' is ambiguous, you don't want
> to error out, but interpret it as a value for the outer one.
I'm not sure.
When exactly are collisions an error? How exactly are non-erroneous
collisions resolved? qapi-code-gen.rst needs to answer this.
Back to the example. If 'type' resolves to ChardevBackend's member, how
should I specify SocketAddressLegacy's member? 'addr.type'?
Aside: existing -chardev infers SocketAddressLegacy's tag addr.type from
the presence of variant members, but that's a separate QAPIfication
problem.
I figure aliases let me refer to these guys at any level I like:
'data.addr.data.FOO', 'data.addr.FOO', 'addr.data.FOO', 'addr.FOO', or
just 'FOO'. Except for 'type', where just 'type' means something else.
Bizarre...
We actually require much less: for QMP chardev-add, we need
'data.addr.data.FOO' and nothing else, and for CLI -chardev, we need
'FOO' and nothing else (I think). The unneeded ones become accidental
parts of the external interface. If experience is any guide, we'll have
plenty of opportunity to regret such accidents :)
Can we somehow restrict external interfaces to what we actually need?
>> > Never skipping wildcard aliases makes the code simpler and results in
>> > the expected error message here. So I'll do that for v4.
>>
>> Trusting your judgement.
>>
>> > Note that parsing something like '--chardev type=socket,addr.type=unix,
>> > path=/tmp/sock,id=c' now depends on the order in the generated code. If
>> > the top level 'type' weren't parsed and removed from the input first,
>> > visiting 'addr.type' would now detect a conflict. For union types, we
>> > know that 'type' is always parsed first, so it's not a problem, but in
>> > the general case you need to be careful with the order.
>>
>> Uff! I think I'd like to understand this better. No need to delay v4
>> for that.
>>
>> Can't yet say whether we need to spell out the limitation in commit
>> message and/or documentation.
>
> The point where we could error out is when parsing SocketAddressLegacy,
> because there can be two possible providers for "type".
>
> The idea in the current code of this series was that we'll just ignore
> wildcard aliases if we already have a value, because then the value must
> be meant for somewhere else. So it doesn't error out and leaves the
> value in the QDict for someone else to pick it up. If nobody picks it
> up, it's an error "'foo' is unexpected".
>
> If we change it and do error out when there are multiple possible values
> through wildcard aliases, then the only thing that makes it work is that
> ChardevBackend.type is parsed first and is therefore not a possible
> value for SocketAddressLegacy.type any more.
You wrote you're picking the alternative with the simpler code for v4.
Fine with me, as long as it's reasonably easy to explain, in
qapi-code-gen.rst.
Am 14.09.2021 um 15:29 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 14.09.2021 um 10:59 hat Markus Armbruster geschrieben:
> >> >> > + /* You can't use more than one option at the same time */
> >> >> > + v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'foo': 42 } }");
> >> >> > + visit_type_AliasStruct3(v, NULL, &tmp, &err);
> >> >> > + error_free_or_abort(&err);
> >> >>
> >> >> "Parameter 'foo' is unexpected". Say what? It *is* expected, it just
> >> >> clashes with 'nested.foo'.
> >> >>
> >> >> I figure this is what happens:
> >> >>
> >> >> * visit_type_AliasStruct3()
> >> >>
> >> >> - visit_start_struct()
> >> >>
> >> >> - visit_type_AliasStruct3_members()
> >> >>
> >> >> • visit_type_AliasStruct1() for member @nested.
> >> >>
> >> >> This consumes consumes input nested.foo.
> >> >>
> >> >> - visit_check_struct()
> >> >>
> >> >> Error: input foo has not been consumed.
> >> >>
> >> >> Any ideas on how to report this error more clearly?
> >> >
> >> > It's a result of the logic that wildcard aliases are silently ignored
> >> > when they aren't needed. The reason why I included this is that it would
> >> > allow you to have two members with the same name in the object
> >> > containing the alias and in the aliased object without conflicting as
> >> > long as both are given.
> >>
> >> *brain cramp*
> >>
> >> Example?
> >
> > Let's use the real-world example I mentioned below:
> >
> > { 'union': 'ChardevBackend',
> > 'data': { ...,
> > 'socket': 'ChardevSocket',
> > ... },
> > 'aliases': [ { 'source': ['data'] } ] }
>
> To pretend the simple union was flat, i.e. peel off its 'data', because
> that nesting doesn't exist in the CLI you want to QAPIfy.
>
> >
> > { 'struct': 'ChardevSocket',
> > 'data': { 'addr': 'SocketAddressLegacy',
> > ... },
> > 'base': 'ChardevCommon',
> > 'aliases': [ { 'source': ['addr'] } ] }
>
> To unbox struct SocketAddressLegacy, i.e. peel off its 'addr', for the
> same reason.
>
> >
> > { 'union': 'SocketAddressLegacy',
> > 'data': {
> > 'inet': 'InetSocketAddress',
> > 'unix': 'UnixSocketAddress',
> > 'vsock': 'VsockSocketAddress',
> > 'fd': 'String' },
> > 'aliases': [
> > { 'source': ['data'] },
>
> To pretend the simple union was flat, i.e. peel off its 'data',
>
> > { 'name': 'fd', 'source': ['data', 'str'] }
>
> To unbox struct String, i.e. peel off its 'data'.
>
> > ] }
>
> Okay, I understand what you're trying to do. However:
>
> > We have two simple unions there, and wildcard aliases all the way
> > through, so that you can have things like "hostname" on the top level.
> > However, two simple unions mean that "type" could refer to either
> > ChardevBackend.type or to SocketAddressLegacy.type.
>
> Yup. In ChardevBackend, we have both a (common) member @type, and a
> chain of aliases that resolves @type to data.addr.data.type.
>
> > Even though strictly speaking 'type=socket' is ambiguous, you don't want
> > to error out, but interpret it as a value for the outer one.
>
> I'm not sure.
It's the only possible syntax for specifying ChardevBackend.type, so if
you don't allow this, everything breaks down.
> When exactly are collisions an error? How exactly are non-erroneous
> collisions resolved? qapi-code-gen.rst needs to answer this.
The strategy implemented in this version is: Collisions are generally an
error, except for wildcard aliases conflicting with a non-wildcard-alias
value. In this case the wildcard alias is ignored and the value is
assumed to belong elsewhere.
If it doesn't belong elsewhere in the end, it still sits in the QDict
when qobject_input_check_struct() runs, so you get an error.
> Back to the example. If 'type' resolves to ChardevBackend's member, how
> should I specify SocketAddressLegacy's member? 'addr.type'?
>
> Aside: existing -chardev infers SocketAddressLegacy's tag addr.type from
> the presence of variant members, but that's a separate QAPIfication
> problem.
So does the new -chardev in its compatibility code, but if you want to
specify it explicitly, addr.type is what you should use.
> I figure aliases let me refer to these guys at any level I like:
> 'data.addr.data.FOO', 'data.addr.FOO', 'addr.data.FOO', 'addr.FOO', or
> just 'FOO'. Except for 'type', where just 'type' means something else.
> Bizarre...
About as bizarre as shadowing variables in C. The more local one wins.
> We actually require much less: for QMP chardev-add, we need
> 'data.addr.data.FOO' and nothing else, and for CLI -chardev, we need
> 'FOO' and nothing else (I think). The unneeded ones become accidental
> parts of the external interface. If experience is any guide, we'll have
> plenty of opportunity to regret such accidents :)
>
> Can we somehow restrict external interfaces to what we actually need?
Not reasonably, I would say. Of course, you could try to cover all
paths with aliases in the top level, but the top level really shouldn't
have to know about the details of types deep inside some union variants.
The solution for reducing the allowed options here is to work on
introspection, mark 'data' deprecated everywhere and get rid of the
simple union nonsense.
> >> > Never skipping wildcard aliases makes the code simpler and results in
> >> > the expected error message here. So I'll do that for v4.
> >>
> >> Trusting your judgement.
> >>
> >> > Note that parsing something like '--chardev type=socket,addr.type=unix,
> >> > path=/tmp/sock,id=c' now depends on the order in the generated code. If
> >> > the top level 'type' weren't parsed and removed from the input first,
> >> > visiting 'addr.type' would now detect a conflict. For union types, we
> >> > know that 'type' is always parsed first, so it's not a problem, but in
> >> > the general case you need to be careful with the order.
> >>
> >> Uff! I think I'd like to understand this better. No need to delay v4
> >> for that.
> >>
> >> Can't yet say whether we need to spell out the limitation in commit
> >> message and/or documentation.
> >
> > The point where we could error out is when parsing SocketAddressLegacy,
> > because there can be two possible providers for "type".
> >
> > The idea in the current code of this series was that we'll just ignore
> > wildcard aliases if we already have a value, because then the value must
> > be meant for somewhere else. So it doesn't error out and leaves the
> > value in the QDict for someone else to pick it up. If nobody picks it
> > up, it's an error "'foo' is unexpected".
> >
> > If we change it and do error out when there are multiple possible values
> > through wildcard aliases, then the only thing that makes it work is that
> > ChardevBackend.type is parsed first and is therefore not a possible
> > value for SocketAddressLegacy.type any more.
>
> You wrote you're picking the alternative with the simpler code for v4.
> Fine with me, as long as it's reasonably easy to explain, in
> qapi-code-gen.rst.
I think I've come to the conclusion that it's not easy enough to
explain. As long as the parsing order should remain an implementation
detail that schema authors shouldn't rely on, it's not possible at all.
It's a pity because the code would have been simpler and it would
probably have worked for the cases we're interested in. But it doesn't
work in all hypothetical cases and we can't document the conditions for
that without making people rely on the parsing order.
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 14.09.2021 um 15:29 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Am 14.09.2021 um 10:59 hat Markus Armbruster geschrieben:
>> >> >> > + /* You can't use more than one option at the same time */
>> >> >> > + v = visitor_input_test_init(data, "{ 'foo': 42, 'nested': { 'foo': 42 } }");
>> >> >> > + visit_type_AliasStruct3(v, NULL, &tmp, &err);
>> >> >> > + error_free_or_abort(&err);
>> >> >>
>> >> >> "Parameter 'foo' is unexpected". Say what? It *is* expected, it just
>> >> >> clashes with 'nested.foo'.
>> >> >>
>> >> >> I figure this is what happens:
>> >> >>
>> >> >> * visit_type_AliasStruct3()
>> >> >>
>> >> >> - visit_start_struct()
>> >> >>
>> >> >> - visit_type_AliasStruct3_members()
>> >> >>
>> >> >> • visit_type_AliasStruct1() for member @nested.
>> >> >>
>> >> >> This consumes consumes input nested.foo.
>> >> >>
>> >> >> - visit_check_struct()
>> >> >>
>> >> >> Error: input foo has not been consumed.
>> >> >>
>> >> >> Any ideas on how to report this error more clearly?
>> >> >
>> >> > It's a result of the logic that wildcard aliases are silently ignored
>> >> > when they aren't needed. The reason why I included this is that it would
>> >> > allow you to have two members with the same name in the object
>> >> > containing the alias and in the aliased object without conflicting as
>> >> > long as both are given.
>> >>
>> >> *brain cramp*
>> >>
>> >> Example?
>> >
>> > Let's use the real-world example I mentioned below:
>> >
>> > { 'union': 'ChardevBackend',
>> > 'data': { ...,
>> > 'socket': 'ChardevSocket',
>> > ... },
>> > 'aliases': [ { 'source': ['data'] } ] }
>>
>> To pretend the simple union was flat, i.e. peel off its 'data', because
>> that nesting doesn't exist in the CLI you want to QAPIfy.
>>
>> >
>> > { 'struct': 'ChardevSocket',
>> > 'data': { 'addr': 'SocketAddressLegacy',
>> > ... },
>> > 'base': 'ChardevCommon',
>> > 'aliases': [ { 'source': ['addr'] } ] }
>>
>> To unbox struct SocketAddressLegacy, i.e. peel off its 'addr', for the
>> same reason.
>>
>> >
>> > { 'union': 'SocketAddressLegacy',
>> > 'data': {
>> > 'inet': 'InetSocketAddress',
>> > 'unix': 'UnixSocketAddress',
>> > 'vsock': 'VsockSocketAddress',
>> > 'fd': 'String' },
>> > 'aliases': [
>> > { 'source': ['data'] },
>>
>> To pretend the simple union was flat, i.e. peel off its 'data',
>>
>> > { 'name': 'fd', 'source': ['data', 'str'] }
>>
>> To unbox struct String, i.e. peel off its 'data'.
>>
>> > ] }
>>
>> Okay, I understand what you're trying to do. However:
>>
>> > We have two simple unions there, and wildcard aliases all the way
>> > through, so that you can have things like "hostname" on the top level.
>> > However, two simple unions mean that "type" could refer to either
>> > ChardevBackend.type or to SocketAddressLegacy.type.
>>
>> Yup. In ChardevBackend, we have both a (common) member @type, and a
>> chain of aliases that resolves @type to data.addr.data.type.
>>
>> > Even though strictly speaking 'type=socket' is ambiguous, you don't want
>> > to error out, but interpret it as a value for the outer one.
>>
>> I'm not sure.
>
> It's the only possible syntax for specifying ChardevBackend.type, so if
> you don't allow this, everything breaks down.
>
>> When exactly are collisions an error? How exactly are non-erroneous
>> collisions resolved? qapi-code-gen.rst needs to answer this.
>
> The strategy implemented in this version is: Collisions are generally an
> error, except for wildcard aliases conflicting with a non-wildcard-alias
> value. In this case the wildcard alias is ignored and the value is
> assumed to belong elsewhere.
Kinds of collisions:
member ordinary alias wildcard alias
member error[1] error[2] member wins[4]
ordinary alias error[3] ordinary wins[4]
wildcard alias ???[5]
[1] Test case duplicate-key demonstrates.
[2] Test case alias-name-conflict demonstrates.
[3] No test case, manual testing results in "alias 'a' collides with
alias 'a'".
[4] Please confirm I got this right.
[5] No test case, manual testing results in no error. What's the
intended behavior?
> If it doesn't belong elsewhere in the end, it still sits in the QDict
> when qobject_input_check_struct() runs, so you get an error.
>
>> Back to the example. If 'type' resolves to ChardevBackend's member, how
>> should I specify SocketAddressLegacy's member? 'addr.type'?
>>
>> Aside: existing -chardev infers SocketAddressLegacy's tag addr.type from
>> the presence of variant members, but that's a separate QAPIfication
>> problem.
>
> So does the new -chardev in its compatibility code, but if you want to
> specify it explicitly, addr.type is what you should use.
>
>> I figure aliases let me refer to these guys at any level I like:
>> 'data.addr.data.FOO', 'data.addr.FOO', 'addr.data.FOO', 'addr.FOO', or
>> just 'FOO'. Except for 'type', where just 'type' means something else.
>> Bizarre...
>
> About as bizarre as shadowing variables in C. The more local one wins.
The part where member 'type' shadows alias 'type' is intentional, and
you're right, it's hardly bizarre, just risks confusion (so does
shadowing in C).
The part where intermediate aliases contaminate the external interface
is an artifact of how we provide the "final" alias. The combination of
these unwanted artifacts with the shadowing feels bizarre to me. Eye of
the beholder, etc., etc.
>> We actually require much less: for QMP chardev-add, we need
>> 'data.addr.data.FOO' and nothing else, and for CLI -chardev, we need
>> 'FOO' and nothing else (I think). The unneeded ones become accidental
>> parts of the external interface. If experience is any guide, we'll have
>> plenty of opportunity to regret such accidents :)
>>
>> Can we somehow restrict external interfaces to what we actually need?
>
> Not reasonably, I would say. Of course, you could try to cover all
> paths with aliases in the top level, but the top level really shouldn't
> have to know about the details of types deep inside some union variants.
>
> The solution for reducing the allowed options here is to work on
> introspection, mark 'data' deprecated everywhere and get rid of the
> simple union nonsense.
Accidental extension of QMP to enable QAPIfication elsewhere would be a
mistake. Elsewhere right now: -chardev.
The knee-jerk short-term solution for QMP is to ignore aliases there
completely. Without introspection, they can't be used seriously anyway.
Of course, we eventually want to use them for evolving QMP, e.g. to
flatten simple unions. The knee-jerk solution sets up another obstacle.
The issue also exists in -chardev with a JSON argument. We can apply
the knee-jerk solution to any JSON-based interface, not just to QMP.
The issue also exists in -chardev with a dotted keys argument. There,
we definitely need the outermost alias (e.g. "host") for compatibility,
and we may want the member ("data.addr.data.host") for symmetry with
JSON. I can't see an argument for exposing the intermediate aliases as
dotted keys, though.
I find the argument "for symmetry with JSON" quite weak. But exposing
the member seems unlikely to create problems later on.
You argue that "the top level really shouldn't have to know about the
details of types deep inside some union variants." That's a valid
argument about the QAPI schema language's support for abstraction. But
the QAPI schema language is means, while the external interfaces are
ends. They come first. A nicer schema language is certainly desirable,
but the niceties shouldn't leak crap into the external interfaces.
Let me work through an example to ground this. Consider chardev-add /
-chardev. Structure of chardev-add's argument:
id: str
backend:
type: enum ChardevBackendKind
data: one of the following, selected by the value of @type:
for socket:
addr:
type: enum SocketAddressType
data: one of the following, selected by the value of @type:
for inet:
host: str
...
...
In contrast, -chardev's argument is flat. To QAPIfy it, we could use
aliases from the root into the object nest:
from type to backend.type
from host to backend.data.addr.data.host
...
We'd certainly design chardev-add's argument differently today, namely
with much less nesting. Say we want to evolve to this structure:
id: str
type: enum ChardevBackendKind
one of the following, selected by the value of @type:
for socket:
addr:
type: enum SocketAddressType
one of the following, selected by the value of @type:
for inet:
host: str
...
...
We obviously need to keep the old structure around for compatibility.
For that, we could use a *different* set of aliases:
from type to backend.type
from addr.host to backend.data.addr.data.host
...
What's the plan for supporting different uses wanting different aliases?
Throw all the aliases together and shake? Then one interface's
requirements will contaminate other interfaces with unwanted aliases.
Getting one interface right is hard enough, having to reason about *all*
QAPI-based interfaces would set us up for failure. And what if we run
into contradictory requirements?
Could we instead tag aliases somehow, pass a tag to the visitor, and
have it ignore aliases with a different tag?
Reminds me of the problem of generating multiple QMPs from a single
schema, e.g. one for qemu-system-FOO, and another one for
qemu-storage-daemon. Inchoate idea: use tags for that somehow. But I
digress.
I'm actually tempted to try how far we can get with just one level of
aliases, i.e. aliases that can only resolve to a member, not to another
alias. I'd expect the code to become quite a bit simpler.
One of the main reasons why this work has such a rough time in review is
that thinking through possible effects make my head explode. I sure
wish I had a stronger head. Weaker effects are commonly easier to get,
though.
I apologize for this wall of text. I've been thinking in writing again.
>> >> > Never skipping wildcard aliases makes the code simpler and results in
>> >> > the expected error message here. So I'll do that for v4.
>> >>
>> >> Trusting your judgement.
>> >>
>> >> > Note that parsing something like '--chardev type=socket,addr.type=unix,
>> >> > path=/tmp/sock,id=c' now depends on the order in the generated code. If
>> >> > the top level 'type' weren't parsed and removed from the input first,
>> >> > visiting 'addr.type' would now detect a conflict. For union types, we
>> >> > know that 'type' is always parsed first, so it's not a problem, but in
>> >> > the general case you need to be careful with the order.
>> >>
>> >> Uff! I think I'd like to understand this better. No need to delay v4
>> >> for that.
>> >>
>> >> Can't yet say whether we need to spell out the limitation in commit
>> >> message and/or documentation.
>> >
>> > The point where we could error out is when parsing SocketAddressLegacy,
>> > because there can be two possible providers for "type".
>> >
>> > The idea in the current code of this series was that we'll just ignore
>> > wildcard aliases if we already have a value, because then the value must
>> > be meant for somewhere else. So it doesn't error out and leaves the
>> > value in the QDict for someone else to pick it up. If nobody picks it
>> > up, it's an error "'foo' is unexpected".
>> >
>> > If we change it and do error out when there are multiple possible values
>> > through wildcard aliases, then the only thing that makes it work is that
>> > ChardevBackend.type is parsed first and is therefore not a possible
>> > value for SocketAddressLegacy.type any more.
>>
>> You wrote you're picking the alternative with the simpler code for v4.
>> Fine with me, as long as it's reasonably easy to explain, in
>> qapi-code-gen.rst.
>
> I think I've come to the conclusion that it's not easy enough to
> explain. As long as the parsing order should remain an implementation
> detail that schema authors shouldn't rely on, it's not possible at all.
>
> It's a pity because the code would have been simpler and it would
> probably have worked for the cases we're interested in. But it doesn't
> work in all hypothetical cases and we can't document the conditions for
> that without making people rely on the parsing order.
The order in which the visitors visit members is not really specified.
The examples in qapi-code-gen.rst show it's in source order, and the
order is quite unlikely to change. So, making it official is not out of
the question.
However, depending on member order for anything feels iffy. I'd like to
be able to shuffle them around without breaking anything.
Thoughts?
Am 17.09.2021 um 10:26 hat Markus Armbruster geschrieben:
> >> When exactly are collisions an error? How exactly are non-erroneous
> >> collisions resolved? qapi-code-gen.rst needs to answer this.
> >
> > The strategy implemented in this version is: Collisions are generally an
> > error, except for wildcard aliases conflicting with a non-wildcard-alias
> > value. In this case the wildcard alias is ignored and the value is
> > assumed to belong elsewhere.
>
> Kinds of collisions:
>
> member ordinary alias wildcard alias
> member error[1] error[2] member wins[4]
> ordinary alias error[3] ordinary wins[4]
> wildcard alias ???[5]
>
> [1] Test case duplicate-key demonstrates.
>
> [2] Test case alias-name-conflict demonstrates.
>
> [3] No test case, manual testing results in "alias 'a' collides with
> alias 'a'".
>
> [4] Please confirm I got this right.
>
> [5] No test case, manual testing results in no error. What's the
> intended behavior?
[5] is going to become "more local wins". In v3 it's "runtime error if
both are specified, except sometimes".
Both [4] and [5] are runtime errors if two values are specified and
there aren't two consumers for them.
> >> We actually require much less: for QMP chardev-add, we need
> >> 'data.addr.data.FOO' and nothing else, and for CLI -chardev, we need
> >> 'FOO' and nothing else (I think). The unneeded ones become accidental
> >> parts of the external interface. If experience is any guide, we'll have
> >> plenty of opportunity to regret such accidents :)
> >>
> >> Can we somehow restrict external interfaces to what we actually need?
> >
> > Not reasonably, I would say. Of course, you could try to cover all
> > paths with aliases in the top level, but the top level really shouldn't
> > have to know about the details of types deep inside some union variants.
> >
> > The solution for reducing the allowed options here is to work on
> > introspection, mark 'data' deprecated everywhere and get rid of the
> > simple union nonsense.
>
> Accidental extension of QMP to enable QAPIfication elsewhere would be a
> mistake. Elsewhere right now: -chardev.
>
> The knee-jerk short-term solution for QMP is to ignore aliases there
> completely. Without introspection, they can't be used seriously anyway.
I would say it's intentional enough. If we can flatten simple unions for
the CLI, why not accept them in QMP, too? (And management tools will
only be happier if they can use the same representation for QMP and
CLI.) I hope that we can get introspection done for 6.2, but even if we
can't, making the case already work shouldn't hurt anyone.
Now you could argue that some aliases to be introduced for -chardev have
no place in QMP because they have no practical use there. But isn't a
consistent QAPI structure on all external interfaces more valuable than
keeping the interface in QMP minimal, but inconsistent with the CLI?
The problem I would generally see with accidental extension of QMP is
that it may restrict future changes for no reason. But if we already
get the restriction because we must stay compatible with the CLI, too,
then this doesn't apply any more.
> Of course, we eventually want to use them for evolving QMP, e.g. to
> flatten simple unions. The knee-jerk solution sets up another obstacle.
>
> The issue also exists in -chardev with a JSON argument. We can apply
> the knee-jerk solution to any JSON-based interface, not just to QMP.
>
> The issue also exists in -chardev with a dotted keys argument. There,
> we definitely need the outermost alias (e.g. "host") for compatibility,
> and we may want the member ("data.addr.data.host") for symmetry with
> JSON. I can't see an argument for exposing the intermediate aliases as
> dotted keys, though.
>
> I find the argument "for symmetry with JSON" quite weak. But exposing
> the member seems unlikely to create problems later on.
Well, my simple argument is: It's hard to get rid of them, so why bother
with extra complexity to get rid of them?
But I think there is a better argument, and "symmetry with JSON"
actually covers support for the intermediate aliases, too:
The alias that flattens SocketAddressLegacy isn't an alias for the
command chardev-add, it's an alias for the type. If you have code that
formats JSON for SocketAddressLegacy, then you should be able to use it
everywhere where a SocketAddressLegacy is required.
So if your code to format JSON for SocketAddressLegacy uses the alias to
provide a flat representation, but the caller producing ChardevBackend
doesn't flatten the union yet, then that should be fine. And if you have
code for flat ChardevBackend, but your common SocketAddressLegacy code
still produces the nesting, then that should be fine, too.
Essentially partial use of aliases in JSON is a feature to allow libvirt
adopting changes incrementally. And just having a mapping of JSON to the
command line is why it should be there in dotted key syntax, too.
> You argue that "the top level really shouldn't have to know about the
> details of types deep inside some union variants." That's a valid
> argument about the QAPI schema language's support for abstraction. But
> the QAPI schema language is means, while the external interfaces are
> ends. They come first. A nicer schema language is certainly desirable,
> but the niceties shouldn't leak crap into the external interfaces.
>
> Let me work through an example to ground this. Consider chardev-add /
> -chardev. Structure of chardev-add's argument:
>
> id: str
> backend:
> type: enum ChardevBackendKind
> data: one of the following, selected by the value of @type:
> for socket:
> addr:
> type: enum SocketAddressType
> data: one of the following, selected by the value of @type:
> for inet:
> host: str
> ...
> ...
>
> In contrast, -chardev's argument is flat. To QAPIfy it, we could use
> aliases from the root into the object nest:
>
> from type to backend.type
> from host to backend.data.addr.data.host
> ...
>
> We'd certainly design chardev-add's argument differently today, namely
> with much less nesting. Say we want to evolve to this structure:
>
> id: str
> type: enum ChardevBackendKind
> one of the following, selected by the value of @type:
> for socket:
> addr:
> type: enum SocketAddressType
> one of the following, selected by the value of @type:
> for inet:
> host: str
> ...
> ...
>
> We obviously need to keep the old structure around for compatibility.
> For that, we could use a *different* set of aliases:
>
> from type to backend.type
> from addr.host to backend.data.addr.data.host
> ...
>
> What's the plan for supporting different uses wanting different aliases?
> Throw all the aliases together and shake? Then one interface's
> requirements will contaminate other interfaces with unwanted aliases.
> Getting one interface right is hard enough, having to reason about *all*
> QAPI-based interfaces would set us up for failure. And what if we run
> into contradictory requirements?
Are there legitimate reasons for exposing the same QAPI type in
different ways on different interfaces? This sounds like a bad idea to
me. If it's the same thing, it should look the same.
The biggest reason for QAPIfying things is to unify interfaces instead
of having different parsers everywhere. Intentionally accepting some
keys only in QMP and others only in the CLI seems to go against this
goal.
> Could we instead tag aliases somehow, pass a tag to the visitor, and
> have it ignore aliases with a different tag?
>
> Reminds me of the problem of generating multiple QMPs from a single
> schema, e.g. one for qemu-system-FOO, and another one for
> qemu-storage-daemon. Inchoate idea: use tags for that somehow. But I
> digress.
>
> I'm actually tempted to try how far we can get with just one level of
> aliases, i.e. aliases that can only resolve to a member, not to another
> alias. I'd expect the code to become quite a bit simpler.
The visitor code would become slightly simpler, but the schema would
become much less maintainable. If someone adds a new field to, say,
InetSocketAddress, review would have to catch this and request that a
new alias be added to ChardevOptions. I don't think this is a realistic
option.
> > I think I've come to the conclusion that it's not easy enough to
> > explain. As long as the parsing order should remain an implementation
> > detail that schema authors shouldn't rely on, it's not possible at all.
> >
> > It's a pity because the code would have been simpler and it would
> > probably have worked for the cases we're interested in. But it doesn't
> > work in all hypothetical cases and we can't document the conditions for
> > that without making people rely on the parsing order.
>
> The order in which the visitors visit members is not really specified.
> The examples in qapi-code-gen.rst show it's in source order, and the
> order is quite unlikely to change. So, making it official is not out of
> the question.
>
> However, depending on member order for anything feels iffy. I'd like to
> be able to shuffle them around without breaking anything.
>
> Thoughts?
No, I agree. As I said it's not simple enough to explain, so I'll just
have the deleted code back and add a bit to improve error reporting. Not
a big deal for me - though it might be one for your review...
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 17.09.2021 um 10:26 hat Markus Armbruster geschrieben:
[...]
>> >> We actually require much less: for QMP chardev-add, we need
>> >> 'data.addr.data.FOO' and nothing else, and for CLI -chardev, we need
>> >> 'FOO' and nothing else (I think). The unneeded ones become accidental
>> >> parts of the external interface. If experience is any guide, we'll have
>> >> plenty of opportunity to regret such accidents :)
>> >>
>> >> Can we somehow restrict external interfaces to what we actually need?
>> >
>> > Not reasonably, I would say. Of course, you could try to cover all
>> > paths with aliases in the top level, but the top level really shouldn't
>> > have to know about the details of types deep inside some union variants.
>> >
>> > The solution for reducing the allowed options here is to work on
>> > introspection, mark 'data' deprecated everywhere and get rid of the
>> > simple union nonsense.
>>
>> Accidental extension of QMP to enable QAPIfication elsewhere would be a
>> mistake. Elsewhere right now: -chardev.
>>
>> The knee-jerk short-term solution for QMP is to ignore aliases there
>> completely. Without introspection, they can't be used seriously anyway.
>
> I would say it's intentional enough. If we can flatten simple unions for
> the CLI, why not accept them in QMP, too? (And management tools will
> only be happier if they can use the same representation for QMP and
> CLI.) I hope that we can get introspection done for 6.2, but even if we
> can't, making the case already work shouldn't hurt anyone.
>
> Now you could argue that some aliases to be introduced for -chardev have
> no place in QMP because they have no practical use there. But isn't a
> consistent QAPI structure on all external interfaces more valuable than
> keeping the interface in QMP minimal, but inconsistent with the CLI?
>
> The problem I would generally see with accidental extension of QMP is
> that it may restrict future changes for no reason. But if we already
> get the restriction because we must stay compatible with the CLI, too,
> then this doesn't apply any more.
I agree consistency matters. But what exactly should be consistent?
I believe we all agree that a CLI option's *JSON* argument should be
consistent with the corresponding QMP command. This is easy: since both
JSON arguments are fed to the same QAPI machinery, consistency is the
default, and inconsistency would take extra effort.
But what about the dotted keys argument?
One point of view is the difference between the JSON and the dotted keys
argument should be concrete syntax only. Fine print: there may be
arguments dotted keys can't express, but let's ignore that here.
Another point of view is that dotted keys arguments are to JSON
arguments what HMP is to QMP: a (hopefully) human-friendly layer on top,
where we have a certain degree of freedom to improve on the
machine-friendly interface for human use.
Let me try to explain why I believe the latter one makes more sense.
When QAPIfying existing CLI options, the new dotted keys option argument
must be backward compatible to the old option argument, and the QMP
command must also remain backward compatible.
If their abstract syntax is already perfectly consistent, we can simply
use qobject_input_visitor_new_str(). All we have to worry then is the
differences between dotted keys syntax and whatever it replaces (almost
always QemuOpts), which is of no concern to us right now.
What if their abstract syntax differs, as it does for -chardev and
QMP chardev-add?
One solution is to use the *union* of the two languages both for CLI and
QMP. Complicates both. I don't like this. We can try to mitigate by
deprecating unwanted parts of the abstract syntax. More on that below.
If we treat dotted keys as separate, human-friendly layer on top, we can
instead *translate* from one language to the other. This is actually
what -chardev, HMP chardev-add, and all the other HMP commands wrapping
around QMP do today, or close to it.
Different tack. I've been struggling with distilling my intuitions
about the proposed QAPI aliases feature into thought (one reason for me
taking so painfully long to reply). I believe my difficulties are in
part due to a disconnect from use cases: there's a lot aliases could
enable us to do, and I keep getting lost in possibilities. So let's try
to ground this by looking at QAPIfication of -chardev. This way, we
discuss how to solve a specific problem in its entirety instead of
trying to come to a conclusion on a building block that may be useful
(but isn't sufficient) to solve an unclear set of problems (unclear to
*me*).
The equivalent QMP command is chardev-add. The definition of its
argument is spread over several QAPI type definitions. It boils down
to:
id: str
backend: ChardevBackend
type: ChardevBackendKind tag
data: ChardevFile when file
*logfile: str
*logappend: bool
*in: str
out: str
*append: bool
...
data: ChardevSocket when socket
*logfile: str
*logappend: bool
addr: SocketAddressLegacy
type: SocketAddressType tag
data: InetSocketAddress when inet
host: str
port: str
*numeric: bool
...
...
...
This is old stuff; we'd use a lot less nesting today.
The argument of -chardev is flat, as output of -help shows:
...
-chardev socket,id=id[,host=host],port=port[,to=to][,ipv4=on|off][,ipv6=on|off][,nodelay=on|off][,reconnect=seconds]
...
-chardev file,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]
...
The historical reason for -chardev being flat is that QemuOpts supports
only flat out of the box. Nested requires gymnastics of the kind we
generally perform only under duress.
There's also a reason for *keeping* it flat: nesting is awful in dotted
keys syntax. With JSON, you simply throw another pair of braces on the
heap. With dotted keys, you get to repeat long key prefixes over and
over.
Let me stitch the two interfaces together:
-chardev file backend.type
,id=id, id
path=path backend.data.out
backend.data.in
backend.data.append
[,mux=on|off]
[,logfile=PATH] backend.data.logfile
[,logappend=on|off] backend.data.logappend
...
-chardev socket backend.type
,id=id id
backend.data.addr.type
[,host=host] backend.data.addr.data.host + default
,port=port backend.data.addr.data.port
backend.data.addr.data.numeric
[,to=to] backend.data.addr.data.to
[,ipv4=on|off] backend.data.addr.data.ipv4
[,ipv6=on|off] backend.data.addr.data.ipv6
backend.data.addr.data.keep-alive
backend.data.addr.data.mptcp
[,nodelay=on|off] backend.data.nodelay
[,reconnect=seconds] backend.data.reconnect
[,server=on|off] backend.data.server
[,wait=on|off] backend.data.wait
[,telnet=on|off] backend.data.telnet
backend.data.tn3270
[,websocket=on|off] backend.data.websocket
[,mux=on|off]
[,logfile=PATH] backend.data.logfile
[,logappend=on|off] backend.data.logappend
[,tls-creds=ID] backend.data.tls-creds
[,tls-authz=ID] backend.data.tls-authz
CLI parameter @mux has nothing in the right column. The code looks like
it's syntactic sugar for something I don't understand. Moving on.
A few things in the right column have nothing in the left column. I can
see three possible cases:
* It's only missing in help. Example: parameter append actually exists,
and corresponds to backend.data.append
* -chardev can't do. Example, maybe: backend.data.in.
* Magic. Example: the value of backend.data.addr.type is derived from
presence of path.
Both -chardev and QMP chardev-add use the same helpers (there are minor
differences that don't matter here). The former basically translates
its flat argument into the latter's arguments. HMP chardev-add is just
like -chardev.
The quickest way to QAPIfy existing -chardev is probably the one we
chose for -object: if the argument is JSON, use the new, QAPI-based
interface, else use the old interface.
Sugar is only available with the old QemuOpts argument.
Feature-parity with QMP is only available with the new JSON argument.
This kicks the "retire QemuOpts" can down the road. The hope is to
eventually weaken the compatibility promise for the non-JSON argument
(like we did for HMP), then switch it to dotted keys. Whether we ditch
the translation from flat to nested then is an open question.
A different problem would be adding a new -chardev somewhere. Not
having to drag all the compatibility baggage along would be nice. On
the other hand, dotted keys without flattening would result in an awful
human interface. Flattening by hand like the existing code does is
unappealing. We could use a bit of help from the QAPI generator.
Perhaps we hate QMP chardev-add's nesting enough to flatten it to
something like
id: str
type: ChardevBackendKind tag
# ChardevFile when file
*logfile: str
*logappend: bool
*in: str
out: str
*append: bool
...
# ChardevSocket when socket
*logfile: str
*logappend: bool
addr: SocketAddress
type: SocketAddressType tag
# InetSocketAddress when inet
host: str
port: str
*numeric: bool
To avoid a compatibility break, we'd actually have to keep the old
structure around, too. Deprecate, drop after grace period expires.
Now the question that matters for this series: how can QAPI aliases help
us with the things discussed above?
1. Aliases can help with flattening input wire formats. Missing parts:
introspection, feature deprecated for aliases.
Note:
* Input: aliases don't affect output.
* Wire format: aliases don't affect the generated C types.
* Aliases can only be "flatter" than the thing they alias. The members
remain nested until drop the old interface.
* Feature deprecated for aliases: straightforward, I think.
* Introspection: requires a schema extension, which means users of
introspection will need an update to actually see aliases.
Corollary: until all of the users that matter are updated, we can't
deprecate members, only aliases. Unfortunately, members is what we
actually *want* to deprecate here.
Why do we need a schema extension? We can't simply expose aliases as
if they were members, because that would look as if you had to specify
both (which is actually an error).
When we drop the old interface, the old, deprecated members go away,
and the aliases morph into members. Well-behaved users of
introspection should deal with the morphing. Still, there's a risk.
As much as I hate excessive nesting, I'm not sure flattening old
interfaces such as QMP chardev-add is worth the trouble. More on that
below, in reply to your arguments.
Flattening could reduce the difference to the current, non-QAPIfied
-chardev, though, which could help with QAPIfying it. How exactly
remains unclear, at least to me, especially for sugar and magic.
2. Aliases can help with adding flatter options to existing input wire
formats. This is 1. less the hope to get rid of the nested option some
day. Missing parts: introspection. Old introspection users simply
can't see the flat option, which is fine.
Is adding a flat option to old interfaces such as QMP chardev-add worth
the trouble? If you have to check whether the flat option is available
before you can use it, and else fall back to the nested option, always
using the nested option is simpler.
I figure the most likely user of these aliases would be a QAPIfied
-chardev. That use is legitimate. But if it remains the only one, all
the extra work to expose them in the external interface in a consumable
way (the "missing parts" above) is wasted.
3. Aliases might help with creating flatter variations of old input wire
formats. This is 2. plus a way to limit each interface to one option.
Missing parts: a way to limit, introspection.
Note:
* "A way to limit" is too vague to be useful. Needs design work.
Stupidest solution that could possibly work: hard-code two options,
with and without aliases.
* Introspection is needed only for machine-friendly interfaces.
Implementing it might be expensive.
If we use the flat variation just internally, say for -chardev's dotted
keys argument, then introspection is not needed. We'd use "with
aliases" just for translating -chardev's dotted keys argument.
We could also use the flat variation to create alternate, more modern
interfaces with much less code duplication in the schema, and much less
boring translation code between the differently nested duplicates.
Example: we have SocketAddress for use by "modern" interfaces, and
SocketAddressLegacy strictly for existing ones. We have hand-written
code to convert SocketAddressLegacy to SocketAddress. Not terrible for
a simple type like SocketAddress, but imagine doing the same for
flattening ChardevBackend.
>> Of course, we eventually want to use them for evolving QMP, e.g. to
>> flatten simple unions. The knee-jerk solution sets up another obstacle.
>>
>> The issue also exists in -chardev with a JSON argument. We can apply
>> the knee-jerk solution to any JSON-based interface, not just to QMP.
>>
>> The issue also exists in -chardev with a dotted keys argument. There,
>> we definitely need the outermost alias (e.g. "host") for compatibility,
>> and we may want the member ("data.addr.data.host") for symmetry with
>> JSON. I can't see an argument for exposing the intermediate aliases as
>> dotted keys, though.
>>
>> I find the argument "for symmetry with JSON" quite weak. But exposing
>> the member seems unlikely to create problems later on.
>
> Well, my simple argument is: It's hard to get rid of them, so why bother
> with extra complexity to get rid of them?
It's either that, or writing documentation explaining the now many ways
to do the same thing.
> But I think there is a better argument, and "symmetry with JSON"
> actually covers support for the intermediate aliases, too:
>
> The alias that flattens SocketAddressLegacy isn't an alias for the
> command chardev-add, it's an alias for the type. If you have code that
> formats JSON for SocketAddressLegacy, then you should be able to use it
> everywhere where a SocketAddressLegacy is required.
>
> So if your code to format JSON for SocketAddressLegacy uses the alias to
> provide a flat representation, but the caller producing ChardevBackend
> doesn't flatten the union yet, then that should be fine. And if you have
> code for flat ChardevBackend, but your common SocketAddressLegacy code
> still produces the nesting, then that should be fine, too.
>
> Essentially partial use of aliases in JSON is a feature to allow libvirt
> adopting changes incrementally.
This is about evolving existing JSON-based interfaces (QMP, basically)
compatibly.
To be able to use aliases for this, we need to supply missing parts
(detailed above), and they are anything but trivial. Users of
introspection need updates, too.
I hate excessive nesting, I really do. I'd love to see excessively
nested existing interfaces cleaned up. Sadly, the paths towards that
goal I can see feel too costly just for cleanliness.
> And just having a mapping of JSON to the
> command line is why it should be there in dotted key syntax, too.
We don't have a mapping from HMP to QMP, only the other way round.
I believe we should make CLI with dotted keys like HMP (not least
because it already is for many complex options). Then we don't have a
mapping from QMP to CLI with dotted keys. We do have a (trivial!)
mapping to CLI with JSON.
>> You argue that "the top level really shouldn't have to know about the
>> details of types deep inside some union variants." That's a valid
>> argument about the QAPI schema language's support for abstraction. But
>> the QAPI schema language is means, while the external interfaces are
>> ends. They come first. A nicer schema language is certainly desirable,
>> but the niceties shouldn't leak crap into the external interfaces.
>>
>> Let me work through an example to ground this. Consider chardev-add /
>> -chardev. Structure of chardev-add's argument:
>>
>> id: str
>> backend:
>> type: enum ChardevBackendKind
>> data: one of the following, selected by the value of @type:
>> for socket:
>> addr:
>> type: enum SocketAddressType
>> data: one of the following, selected by the value of @type:
>> for inet:
>> host: str
>> ...
>> ...
>>
>> In contrast, -chardev's argument is flat. To QAPIfy it, we could use
>> aliases from the root into the object nest:
>>
>> from type to backend.type
>> from host to backend.data.addr.data.host
>> ...
>>
>> We'd certainly design chardev-add's argument differently today, namely
>> with much less nesting. Say we want to evolve to this structure:
>>
>> id: str
>> type: enum ChardevBackendKind
>> one of the following, selected by the value of @type:
>> for socket:
>> addr:
>> type: enum SocketAddressType
>> one of the following, selected by the value of @type:
>> for inet:
>> host: str
>> ...
>> ...
>>
>> We obviously need to keep the old structure around for compatibility.
>> For that, we could use a *different* set of aliases:
>>
>> from type to backend.type
>> from addr.host to backend.data.addr.data.host
>> ...
>>
>> What's the plan for supporting different uses wanting different aliases?
>> Throw all the aliases together and shake? Then one interface's
>> requirements will contaminate other interfaces with unwanted aliases.
>> Getting one interface right is hard enough, having to reason about *all*
>> QAPI-based interfaces would set us up for failure. And what if we run
>> into contradictory requirements?
>
> Are there legitimate reasons for exposing the same QAPI type in
> different ways on different interfaces? This sounds like a bad idea to
> me. If it's the same thing, it should look the same.
>
> The biggest reason for QAPIfying things is to unify interfaces instead
> of having different parsers everywhere. Intentionally accepting some
> keys only in QMP and others only in the CLI seems to go against this
> goal.
Valid argument. CLI with JSON argument should match QMP. Even when
that means both are excessively nested.
CLI with dotted keys is a different matter, in my opinion.
>> Could we instead tag aliases somehow, pass a tag to the visitor, and
>> have it ignore aliases with a different tag?
>>
>> Reminds me of the problem of generating multiple QMPs from a single
>> schema, e.g. one for qemu-system-FOO, and another one for
>> qemu-storage-daemon. Inchoate idea: use tags for that somehow. But I
>> digress.
>>
>> I'm actually tempted to try how far we can get with just one level of
>> aliases, i.e. aliases that can only resolve to a member, not to another
>> alias. I'd expect the code to become quite a bit simpler.
>
> The visitor code would become slightly simpler, but the schema would
> become much less maintainable. If someone adds a new field to, say,
> InetSocketAddress, review would have to catch this and request that a
> new alias be added to ChardevOptions. I don't think this is a realistic
> option.
Shouldn't a wildcard alias in ChardevOptions take care of that?
[...]
I apologize for this wall of text. It's a desparate attempt to cut
through the complexity and my confusion, and make sense of the actual
problems we're trying to solve.
So, what problems exactly are we trying to solve?
Am 02.10.2021 um 15:33 hat Markus Armbruster geschrieben: > I apologize for this wall of text. It's a desparate attempt to cut > through the complexity and my confusion, and make sense of the actual > problems we're trying to solve. > > So, what problems exactly are we trying to solve? I'll start with replying to your final question because I think it's more helpful to start with the big picture than with details. So tools like libvirt want to have a single consistent interface to configure things on startup and at runtime. We also intend to support configuration files that should this time support all of the options and not just a few chosen ones. The hypothesis is that QAPIfying the command line is the correct solution for both of these problems, i.e. all available command line options must be present in the QAPI schema and will be processed by a single parser shared with QMP to make sure they are consistent. Adding QAPIfied versions of individual command line options are steps towards this goal. As soon as they exist for every option, the final conversion from an open coded getopt() loop (or in fact a hand crafted parser in the case of vl.c) to something generated from the QAPI schema should be reasonably easy. You're right that adding a second JSON-based command line interface for every option can already achieve the goal of providing a unified external interface, at the cost of (interface and code) duplication. Is this duplication desirable? Certainly no. Is it acceptable? You might get different opinions on this one. In my opinion, we should try to get rid of hand crafted parsers where it's reasonably possible, and take advantage of the single unified option structure that QAPI provides. -chardev specifically has a hand crafted parser that essentially duplicates the automatically generated QAPI visitor code, except for the nesting and some differences in option names. Aliases are one tool that can help bridge these differences in a generic way with minimal effort in the individual case. They are not _necessary_ to solve the problem; we could instead just use manually written code to manipulate input QDicts so that QAPI visitors accept them. Even with aliases, there are a few things left in the chardev code that are converted this way. Aliases just greatly reduce the amount of this code and make the conversion declarative instead. Now a key point in the previous paragraph is that aliases add a generic way to do this. So even if they are immediately motivated by -chardev, it might be worth looking at other cases they could enable if you think that -chardev alone isn't sufficient justification to have this tool. I guess this is the point where things become a bit less clear because people are just hand waving with vague ideas for additional uses. Do we need to invest more thought on these other cases? We probably do if it makes a difference for the answer to the question whether we want to add aliases to our toolbox. Does it? > But what about the dotted keys argument? > > One point of view is the difference between the JSON and the dotted keys > argument should be concrete syntax only. Fine print: there may be > arguments dotted keys can't express, but let's ignore that here. > > Another point of view is that dotted keys arguments are to JSON > arguments what HMP is to QMP: a (hopefully) human-friendly layer on top, > where we have a certain degree of freedom to improve on the > machine-friendly interface for human use. This doesn't feel unreasonable because with HMP, there is precedence. But this is not all what we have used dotted key syntax for so far. I'm not against making a change there if we feel it makes sense, but we should be clear that it is a change. I think we all agree that -blockdev isn't human-friendly. Dotted key syntax makes it humanly possible to use it on the command line, but friendly is something else entirely. It is still a 1:1 mapping of QMP to the command line, and this is how we advertised it, too. This has some important advantages, like we don't have a separate parser for a human-friendly syntax, but the generic keyval parser covers it. HMP in contrast means separate code for every command, or translated to the CLI for each command line option. HMP is not defined in QAPI, it's a separate thing that just happens to call into QAPI-based QMP code in the common case. Is this what you have in mind for non-JSON command line options? What I had in mind was using the schema to generate the necessary code, using the generic keyval parser everywhere, and just providing a hook where the QDict could be modified after the keyval parser and before the visitor. Most command line options would not have any explicit code for parsing input, only the declarative schema and the final option handler getting a QAPI type (which could actually be the corresponding QMP command handler in the common case). I think these are very different ideas. If we want HMP-like, then all the keyval based code paths we've built so far don't make much sense. > Both -chardev and QMP chardev-add use the same helpers (there are minor > differences that don't matter here). The former basically translates > its flat argument into the latter's arguments. HMP chardev-add is just > like -chardev. > > The quickest way to QAPIfy existing -chardev is probably the one we > chose for -object: if the argument is JSON, use the new, QAPI-based > interface, else use the old interface. -chardev doesn't quite translate into chardev-add arguments. Converting into the input of a QMP command normally means providing a QDict that can be accepted by the QAPI-generated visitor code, and then using that QAPI parser to create the C object. What -chardev does instead is using an entirely separate parser to create the C object directly, without going through any QAPI code. After QAPIfication, both code paths go through the QAPI code and undergo the same validations etc. If we still have code paths that completely bypass QAPI, it's hard to call the command line option fully QAPIfied. Of course, if you don't care about duplication and inconsistencies between the interfaces, though, and just want to achieve the initially stated goal of providing at least one interface consistent between QMP and CLI (besides others) and a config file, then it might be QAPIfied enough. > Now the question that matters for this series: how can QAPI aliases > help us with the things discussed above? The translation needs to be implemented somehow. The obvious way is just writing, reviewing and maintaining code that manually translates. (We don't have this code yet for a fully QAPIfied -chardev. We only have code that bypasses QAPI, i.e. translates to the wrong target format.) Aliases help because they allow reducing the amount of code in favour of data, the alias declarations in the schema. > If we use the flat variation just internally, say for -chardev's dotted > keys argument, then introspection is not needed. We'd use "with > aliases" just for translating -chardev's dotted keys argument. Note that we're doing more translations that just flattening with aliases, some options have different names in QMP and the CLI. But either way, yes, alias support could be optional so that you need to explicitly enable it when creating a visitor, and then only the CLI code could actually enable it. Limits the possible future uses for the new tool in our toolbox, but is sufficient for -chardev. We can always extend support for it later if/when we actually want to make use of it outside of the CLI. > Valid argument. CLI with JSON argument should match QMP. Even when > that means both are excessively nested. > > CLI with dotted keys is a different matter, in my opinion. I'm skipping quite a bit of text here, mainly because I think we need an answer first if we really want to switch the keyval options to be HMP-like in more than just the support status (see above). Obviously, the other relevant question would then be if it also ends up like HMP in that most interesting functionality isn't even available and you're forced to use QMP/JSON when you're serious. I guess I can go into more details once we have answered these fundamental questions. (We could and should have talked about them and about whether you want to have aliases at all a year ago, before going into detailed code review and making error messages perfect in code that we might throw away anyway...) Kevin
Kevin Wolf <kwolf@redhat.com> writes:
[...]
> What I had in mind was using the schema to generate the necessary code,
> using the generic keyval parser everywhere, and just providing a hook
> where the QDict could be modified after the keyval parser and before the
> visitor. Most command line options would not have any explicit code for
> parsing input, only the declarative schema and the final option handler
> getting a QAPI type (which could actually be the corresponding QMP
> command handler in the common case).
A walk to the bakery made me see a problem with transforming the qdict
between keyval parsing and the visitor: error reporting. On closer
investigation, the problem exists even with just aliases.
All experiments performed with your complete QAPIfication at
https://repo.or.cz/qemu/kevin.git qapi-alias-chardev-v4.
Example: flattening leads to suboptimal error
$ qemu-system-x86_64 -chardev udp,id=chr0,port=12345,ipv4=om
qemu-system-x86_64: -chardev udp,id=chr0,port=12345,ipv4=om: Parameter 'backend.data.remote.data.ipv4' expects 'on' or 'off'
We're using "alternate" notation, but the error message barks back in
"standard" notation. It comes from the visitor. While less than
pleasant, it's still understandable, because the "standard" key ends
with the "alternate" key.
Example: renaming leads to confusing error
So far, we rename only members of type 'str', where the visitor can't
fail. Just for illustrating the problem, I'm adding one where it can:
diff --git a/qapi/char.json b/qapi/char.json
index 0e39840d4f..b436d83cbe 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -398,7 +398,8 @@
##
{ 'struct': 'ChardevRingbuf',
'data': { '*size': 'int' },
- 'base': 'ChardevCommon' }
+ 'base': 'ChardevCommon',
+ 'aliases': [ { 'name': 'capacity', 'source': [ 'size' ] } ] }
##
# @ChardevQemuVDAgent:
With this patch:
$ qemu-system-x86_64 -chardev ringbuf,id=chr0,capacity=lots
qemu-system-x86_64: -chardev ringbuf,id=chr0,capacity=lots: Parameter 'backend.data.size' expects integer
The error message fails to connect to the offending key=value.
Example: manual transformation leads to confusion #1
Starting point:
$ qemu-system-x86_64 -chardev udp,id=chr0,port=12345,localaddr=localhost
Works. host defaults to localhost, localport defaults to 0, same as in
git master.
Now "forget" to specify @port:
$ qemu-system-x86_64 -chardev udp,id=chr0,localaddr=localhost
qemu-system-x86_64: -chardev udp,id=chr0,localaddr=localhost: Parameter 'backend.data.remote.data.port' is missing
Again, the visitor's error message uses "standard" notation.
We obediently do what the error message tells us to do:
$ qemu-system-x86_64 -chardev udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345
qemu-system-x86_64: -chardev udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345: Parameters 'backend.*' used inconsistently
Mission accomplished: confusion :)
Example: manual transformation leads to confusion #2
Confusion is possible even without tricking the user into mixing
"standard" and "alternate" explicitly:
$ qemu-system-x86_64 -chardev udp,id=chr0,backend.data.remote.type=udp
qemu-system-x86_64: -chardev udp,id=chr0,backend.data.remote.type=udp: Parameters 'backend.*' used inconsistently
Here, the user tried to stick to "standard", but forgot to specify a
required member. The transformation machinery then "helpfully"
transformed nothing into something, which made the visitor throw up.
Clear error reporting is a critical part of a human-friendly interface.
We have two separate problems with it:
1. The visitor reports errors as if aliases didn't exist
Fixing this is "merely" a matter of tracing back alias applications.
More complexity...
2. The visitor reports errors as if manual transformation didn't exist
Manual transformation can distort the users input beyond recognition.
The visitor reports errors for the transformed input.
To fix this one, we'd have to augment the parse tree so it points
back at the actual user input, and then make the manual
transformations preserve that. Uff!
I'm afraid I need another round of thinking on how to best drag legacy
syntax along when we QAPIfy.
[...]
Am 11.10.2021 um 09:44 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> [...]
>
> > What I had in mind was using the schema to generate the necessary code,
> > using the generic keyval parser everywhere, and just providing a hook
> > where the QDict could be modified after the keyval parser and before the
> > visitor. Most command line options would not have any explicit code for
> > parsing input, only the declarative schema and the final option handler
> > getting a QAPI type (which could actually be the corresponding QMP
> > command handler in the common case).
>
> A walk to the bakery made me see a problem with transforming the qdict
> between keyval parsing and the visitor: error reporting. On closer
> investigation, the problem exists even with just aliases.
I already commented on part of this on IRC, but let me reply here as
well.
On general remark is that I would make the same distinction between
aliases for compatibility and usability that I mentioned elsewhere in
this thread.
In the case of compatibility, it's already a concession that we still
accept the option - suboptimal error messages for incorrect command
lines aren't a major concern for me there. Users are supposed to move to
the new syntax anyway.
On the other hand, aliases we employ for usability are cases where we
don't expect humans to move to something else. I think this is mostly
for flattening structures. Here error messages should be good.
> All experiments performed with your complete QAPIfication at
> https://repo.or.cz/qemu/kevin.git qapi-alias-chardev-v4.
>
>
> Example: flattening leads to suboptimal error
>
> $ qemu-system-x86_64 -chardev udp,id=chr0,port=12345,ipv4=om
> qemu-system-x86_64: -chardev udp,id=chr0,port=12345,ipv4=om: Parameter 'backend.data.remote.data.ipv4' expects 'on' or 'off'
>
> We're using "alternate" notation, but the error message barks back in
> "standard" notation. It comes from the visitor. While less than
> pleasant, it's still understandable, because the "standard" key ends
> with the "alternate" key.
This is not a fundamental problem with aliases. The right name for the
option is unambiguous and known to the visitor: It's the name that the
user specified.
With manual QDict modification it becomes a more fundamental problem
because the visitor can't know the original name any more.
>
> Example: renaming leads to confusing error
>
> So far, we rename only members of type 'str', where the visitor can't
> fail. Just for illustrating the problem, I'm adding one where it can:
>
> diff --git a/qapi/char.json b/qapi/char.json
> index 0e39840d4f..b436d83cbe 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -398,7 +398,8 @@
> ##
> { 'struct': 'ChardevRingbuf',
> 'data': { '*size': 'int' },
> - 'base': 'ChardevCommon' }
> + 'base': 'ChardevCommon',
> + 'aliases': [ { 'name': 'capacity', 'source': [ 'size' ] } ] }
>
> ##
> # @ChardevQemuVDAgent:
>
> With this patch:
>
> $ qemu-system-x86_64 -chardev ringbuf,id=chr0,capacity=lots
> qemu-system-x86_64: -chardev ringbuf,id=chr0,capacity=lots: Parameter 'backend.data.size' expects integer
>
> The error message fails to connect to the offending key=value.
Same problem as above. The error message should use the key that the
user actually specified.
>
> Example: manual transformation leads to confusion #1
>
> Starting point:
>
> $ qemu-system-x86_64 -chardev udp,id=chr0,port=12345,localaddr=localhost
>
> Works. host defaults to localhost, localport defaults to 0, same as in
> git master.
>
> Now "forget" to specify @port:
>
> $ qemu-system-x86_64 -chardev udp,id=chr0,localaddr=localhost
> qemu-system-x86_64: -chardev udp,id=chr0,localaddr=localhost: Parameter 'backend.data.remote.data.port' is missing
>
> Again, the visitor's error message uses "standard" notation.
The output isn't wrong, it's just more verbose than necessary.
Getting this one shortened is a bit harder because the right name is
ambiguous, the user didn't specify anything we can just print back.
Possibly in CLI context, making use of any wildcard aliases would be a
reasonable strategy. This would reduce this one to just 'port'.
> We obediently do what the error message tells us to do:
>
> $ qemu-system-x86_64 -chardev udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345
> qemu-system-x86_64: -chardev udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345: Parameters 'backend.*' used inconsistently
>
> Mission accomplished: confusion :)
This one already fails before aliases do their work. The reason is that
the default key for -chardev is 'backend', and QMP and CLI use the same
name 'backend' for two completely different things.
We could rename the default key into 'x-backend' and make it behave the
same as 'backend', then the keyval parser would only fail when you
explicitly wrote '-chardev backend=udp,...' and the problem is more
obvious.
By the way, we have a very similar issue with '-drive file=...', if we
ever want to parse that one with the keyval parser. It can be both a
string for the filename or an object for the configuration of the 'file'
child that many block drivers have.
> Example: manual transformation leads to confusion #2
>
> Confusion is possible even without tricking the user into mixing
> "standard" and "alternate" explicitly:
>
> $ qemu-system-x86_64 -chardev udp,id=chr0,backend.data.remote.type=udp
> qemu-system-x86_64: -chardev udp,id=chr0,backend.data.remote.type=udp: Parameters 'backend.*' used inconsistently
>
> Here, the user tried to stick to "standard", but forgot to specify a
> required member. The transformation machinery then "helpfully"
> transformed nothing into something, which made the visitor throw up.
Not the visitor, but the keyval parser. Same problem as above.
> Clear error reporting is a critical part of a human-friendly interface.
> We have two separate problems with it:
>
> 1. The visitor reports errors as if aliases didn't exist
>
> Fixing this is "merely" a matter of tracing back alias applications.
> More complexity...
>
> 2. The visitor reports errors as if manual transformation didn't exist
>
> Manual transformation can distort the users input beyond recognition.
> The visitor reports errors for the transformed input.
>
> To fix this one, we'd have to augment the parse tree so it points
> back at the actual user input, and then make the manual
> transformations preserve that. Uff!
Manual transformations are hard to write in a way that they give perfect
results. As long as they are only used for legacy syntax and we expect
users to move to a new way to spell things, this might be acceptable for
a transition period until we remove the old syntax.
In other cases, the easiest way is probably to duplicate even more of
the schema and manually make sure that the visitor will accept the input
before we transform it.
The best way to avoid this is providing tools in QAPI that make manual
transformations unnecessary.
> I'm afraid I need another round of thinking on how to best drag legacy
> syntax along when we QAPIfy.
Let me know when you've come to any conclusions (or more things to
discuss).
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 11.10.2021 um 09:44 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> [...]
>>
>> > What I had in mind was using the schema to generate the necessary code,
>> > using the generic keyval parser everywhere, and just providing a hook
>> > where the QDict could be modified after the keyval parser and before the
>> > visitor. Most command line options would not have any explicit code for
>> > parsing input, only the declarative schema and the final option handler
>> > getting a QAPI type (which could actually be the corresponding QMP
>> > command handler in the common case).
>>
>> A walk to the bakery made me see a problem with transforming the qdict
>> between keyval parsing and the visitor: error reporting. On closer
>> investigation, the problem exists even with just aliases.
>
> I already commented on part of this on IRC, but let me reply here as
> well.
>
> On general remark is that I would make the same distinction between
> aliases for compatibility and usability that I mentioned elsewhere in
> this thread.
>
> In the case of compatibility, it's already a concession that we still
> accept the option - suboptimal error messages for incorrect command
> lines aren't a major concern for me there. Users are supposed to move to
> the new syntax anyway.
Well, these aren't "incorrect", merely deprecated. Bad UX is still bad
there, but at least it'll "fix" itself when the deprecated part goes
away.
> On the other hand, aliases we employ for usability are cases where we
> don't expect humans to move to something else. I think this is mostly
> for flattening structures. Here error messages should be good.
>
>> All experiments performed with your complete QAPIfication at
>> https://repo.or.cz/qemu/kevin.git qapi-alias-chardev-v4.
>>
>>
>> Example: flattening leads to suboptimal error
>>
>> $ qemu-system-x86_64 -chardev udp,id=chr0,port=12345,ipv4=om
>> qemu-system-x86_64: -chardev udp,id=chr0,port=12345,ipv4=om: Parameter 'backend.data.remote.data.ipv4' expects 'on' or 'off'
>>
>> We're using "alternate" notation, but the error message barks back in
>> "standard" notation. It comes from the visitor. While less than
>> pleasant, it's still understandable, because the "standard" key ends
>> with the "alternate" key.
>
> This is not a fundamental problem with aliases. The right name for the
> option is unambiguous and known to the visitor: It's the name that the
> user specified.
This is "1. The visitor reports errors as if aliases didn't exist"
below.
> With manual QDict modification it becomes a more fundamental problem
> because the visitor can't know the original name any more.
This is "2. The visitor reports errors as if manual transformation
didn't exist" below.
I agree 1 is easier to fix than 2.
>> Example: renaming leads to confusing error
>>
>> So far, we rename only members of type 'str', where the visitor can't
>> fail. Just for illustrating the problem, I'm adding one where it can:
>>
>> diff --git a/qapi/char.json b/qapi/char.json
>> index 0e39840d4f..b436d83cbe 100644
>> --- a/qapi/char.json
>> +++ b/qapi/char.json
>> @@ -398,7 +398,8 @@
>> ##
>> { 'struct': 'ChardevRingbuf',
>> 'data': { '*size': 'int' },
>> - 'base': 'ChardevCommon' }
>> + 'base': 'ChardevCommon',
>> + 'aliases': [ { 'name': 'capacity', 'source': [ 'size' ] } ] }
>>
>> ##
>> # @ChardevQemuVDAgent:
>>
>> With this patch:
>>
>> $ qemu-system-x86_64 -chardev ringbuf,id=chr0,capacity=lots
>> qemu-system-x86_64: -chardev ringbuf,id=chr0,capacity=lots: Parameter 'backend.data.size' expects integer
>>
>> The error message fails to connect to the offending key=value.
>
> Same problem as above. The error message should use the key that the
> user actually specified.
Yes.
>> Example: manual transformation leads to confusion #1
>>
>> Starting point:
>>
>> $ qemu-system-x86_64 -chardev udp,id=chr0,port=12345,localaddr=localhost
>>
>> Works. host defaults to localhost, localport defaults to 0, same as in
>> git master.
>>
>> Now "forget" to specify @port:
>>
>> $ qemu-system-x86_64 -chardev udp,id=chr0,localaddr=localhost
>> qemu-system-x86_64: -chardev udp,id=chr0,localaddr=localhost: Parameter 'backend.data.remote.data.port' is missing
>>
>> Again, the visitor's error message uses "standard" notation.
>
> The output isn't wrong, it's just more verbose than necessary.
>
> Getting this one shortened is a bit harder because the right name is
> ambiguous, the user didn't specify anything we can just print back.
>
> Possibly in CLI context, making use of any wildcard aliases would be a
> reasonable strategy. This would reduce this one to just 'port'.
Which of the possible keys we use in the error message boils down to
picking a preferred key.
>> We obediently do what the error message tells us to do:
>>
>> $ qemu-system-x86_64 -chardev udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345
>> qemu-system-x86_64: -chardev udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345: Parameters 'backend.*' used inconsistently
>>
>> Mission accomplished: confusion :)
>
> This one already fails before aliases do their work. The reason is that
> the default key for -chardev is 'backend', and QMP and CLI use the same
> name 'backend' for two completely different things.
Right. I was confused (and the mission was truly accomplished).
> We could rename the default key into 'x-backend' and make it behave the
> same as 'backend', then the keyval parser would only fail when you
> explicitly wrote '-chardev backend=udp,...' and the problem is more
> obvious.
Technically a compatibility break, but we can hope that the longhand
form we change isn't used.
> By the way, we have a very similar issue with '-drive file=...', if we
> ever want to parse that one with the keyval parser. It can be both a
> string for the filename or an object for the configuration of the 'file'
> child that many block drivers have.
Should I be running for the hills?
>> Example: manual transformation leads to confusion #2
>>
>> Confusion is possible even without tricking the user into mixing
>> "standard" and "alternate" explicitly:
>>
>> $ qemu-system-x86_64 -chardev udp,id=chr0,backend.data.remote.type=udp
>> qemu-system-x86_64: -chardev udp,id=chr0,backend.data.remote.type=udp: Parameters 'backend.*' used inconsistently
>>
>> Here, the user tried to stick to "standard", but forgot to specify a
>> required member. The transformation machinery then "helpfully"
>> transformed nothing into something, which made the visitor throw up.
>
> Not the visitor, but the keyval parser. Same problem as above.
>
>> Clear error reporting is a critical part of a human-friendly interface.
>> We have two separate problems with it:
>>
>> 1. The visitor reports errors as if aliases didn't exist
>>
>> Fixing this is "merely" a matter of tracing back alias applications.
>> More complexity...
>>
>> 2. The visitor reports errors as if manual transformation didn't exist
>>
>> Manual transformation can distort the users input beyond recognition.
>> The visitor reports errors for the transformed input.
>>
>> To fix this one, we'd have to augment the parse tree so it points
>> back at the actual user input, and then make the manual
>> transformations preserve that. Uff!
>
> Manual transformations are hard to write in a way that they give perfect
> results. As long as they are only used for legacy syntax and we expect
> users to move to a new way to spell things, this might be acceptable for
> a transition period until we remove the old syntax.
Valid point.
> In other cases, the easiest way is probably to duplicate even more of
> the schema and manually make sure that the visitor will accept the input
> before we transform it.
>
> The best way to avoid this is providing tools in QAPI that make manual
> transformations unnecessary.
Reducing the amount of handwritten code is good, as long as the price is
reasonable. Complex code fetches a higher price.
I think there are a couple of ways to skin this cat.
0. Common to all ways: specify "standard" in the QAPI schema. This
specifies both the "standard" wire format, its parse tree
(represented as QObject), and the "standard" C interface (C types,
basically).
Generic parsers parse into the parse tree. The appropriate input
visitor validates and converts to C types.
1. Parse "alternate" by any means (QemuOpts, keyval, ad hoc), validate,
then transform for the "standard" C interface. Parsing and
validating can fail, the transformation can't.
Drawbacks:
* We duplicate validation, which is a standing invitation for bugs.
* Longwinded, handwritten transformation code. Less vulnerable to
bugs than validation code, I believe.
2. Parse "alternate" by any means (QemuOpts, keyval, ad hoc), transform
to "standard" parse tree.
Drawbacks:
* Bad error messages from input visitor.
* The handwritten transformation code is more longwinded than with 1.
3. Specify "alternate" in the QAPI schema. Map "alternate" C interface
to the "standard" one.
Drawbacks:
* QAPI schema code duplication
* Longwinded, handwritten mapping code.
This is what we did with SocketAddressLegacy.
4. Extend the QAPI schema language to let us specify non-standard wire
format(s). Use that to get the "alternate" wire format we need.
Drawbacks:
* QAPI schema language and generator become more complex.
* Input visitor(s) become more complex.
* Unless we accept "alternate" everywhere, we need a way to limit it
to where it's actually needed. More complexity.
The concrete proposal on the table is aliases. They are not a
complete solution. To solve the whole problem, we combine them with
2. We accept 4's drawbacks for a (sizable) reduction of 2's.
Additionally, we accept "ghost" aliases.
5. Extend the QAPI schema language to let us specify "alternate" wire
formats for "standard" types
This is like 3, except the schema code is mostly referenced instead
duplicated, and the mapping code is generated. Something like
"derive type T' from T with these members moved / renamed".
Drawbacks
* QAPI schema language and generator become more complex.
* Unlike 4, we don't have working code.
Like 4, this will likely solve only part of the problem.
Which one is the least bad?
If this was just about dragging deprecated interfaces to the end of
their grace period, I'd say "whatever is the least work", and that's
almost certainly whatever we have now, possibly hacked up to use the
appropriate internal interface.
Unfortunately, it is also about providing a friendlier interface to
humans "forever".
With 4 or 5, we invest into infrastructure once, then maintain it
forever, to make solving the problem easier and the result easier to
maintain.
Whether the investment will pay off depends on how big and bad the
problem is. Hard to say. One of the reasons we're looking at -chardev
now is that we believe it's the worst of the bunch. But how big is the
bunch, and how bad are the others in there?
>> I'm afraid I need another round of thinking on how to best drag legacy
>> syntax along when we QAPIfy.
>
> Let me know when you've come to any conclusions (or more things to
> discuss).
Is 3 too awful to contemplate?
Markus Armbruster <armbru@redhat.com> writes:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 11.10.2021 um 09:44 hat Markus Armbruster geschrieben:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>> [...]
>>>
>>> > What I had in mind was using the schema to generate the necessary code,
>>> > using the generic keyval parser everywhere, and just providing a hook
>>> > where the QDict could be modified after the keyval parser and before the
>>> > visitor. Most command line options would not have any explicit code for
>>> > parsing input, only the declarative schema and the final option handler
>>> > getting a QAPI type (which could actually be the corresponding QMP
>>> > command handler in the common case).
>>>
>>> A walk to the bakery made me see a problem with transforming the qdict
>>> between keyval parsing and the visitor: error reporting. On closer
>>> investigation, the problem exists even with just aliases.
>>
>> I already commented on part of this on IRC, but let me reply here as
>> well.
>>
>> On general remark is that I would make the same distinction between
>> aliases for compatibility and usability that I mentioned elsewhere in
>> this thread.
>>
>> In the case of compatibility, it's already a concession that we still
>> accept the option - suboptimal error messages for incorrect command
>> lines aren't a major concern for me there. Users are supposed to move to
>> the new syntax anyway.
>
> Well, these aren't "incorrect", merely deprecated. Bad UX is still bad
> there, but at least it'll "fix" itself when the deprecated part goes
> away.
>
>> On the other hand, aliases we employ for usability are cases where we
>> don't expect humans to move to something else. I think this is mostly
>> for flattening structures. Here error messages should be good.
>>
>>> All experiments performed with your complete QAPIfication at
>>> https://repo.or.cz/qemu/kevin.git qapi-alias-chardev-v4.
>>>
>>>
>>> Example: flattening leads to suboptimal error
>>>
>>> $ qemu-system-x86_64 -chardev udp,id=chr0,port=12345,ipv4=om
>>> qemu-system-x86_64: -chardev udp,id=chr0,port=12345,ipv4=om: Parameter 'backend.data.remote.data.ipv4' expects 'on' or 'off'
>>>
>>> We're using "alternate" notation, but the error message barks back in
>>> "standard" notation. It comes from the visitor. While less than
>>> pleasant, it's still understandable, because the "standard" key ends
>>> with the "alternate" key.
>>
>> This is not a fundamental problem with aliases. The right name for the
>> option is unambiguous and known to the visitor: It's the name that the
>> user specified.
>
> This is "1. The visitor reports errors as if aliases didn't exist"
> below.
>
>> With manual QDict modification it becomes a more fundamental problem
>> because the visitor can't know the original name any more.
>
> This is "2. The visitor reports errors as if manual transformation
> didn't exist" below.
>
> I agree 1 is easier to fix than 2.
>
>>> Example: renaming leads to confusing error
>>>
>>> So far, we rename only members of type 'str', where the visitor can't
>>> fail. Just for illustrating the problem, I'm adding one where it can:
>>>
>>> diff --git a/qapi/char.json b/qapi/char.json
>>> index 0e39840d4f..b436d83cbe 100644
>>> --- a/qapi/char.json
>>> +++ b/qapi/char.json
>>> @@ -398,7 +398,8 @@
>>> ##
>>> { 'struct': 'ChardevRingbuf',
>>> 'data': { '*size': 'int' },
>>> - 'base': 'ChardevCommon' }
>>> + 'base': 'ChardevCommon',
>>> + 'aliases': [ { 'name': 'capacity', 'source': [ 'size' ] } ] }
>>>
>>> ##
>>> # @ChardevQemuVDAgent:
>>>
>>> With this patch:
>>>
>>> $ qemu-system-x86_64 -chardev ringbuf,id=chr0,capacity=lots
>>> qemu-system-x86_64: -chardev ringbuf,id=chr0,capacity=lots: Parameter 'backend.data.size' expects integer
>>>
>>> The error message fails to connect to the offending key=value.
>>
>> Same problem as above. The error message should use the key that the
>> user actually specified.
>
> Yes.
>
>>> Example: manual transformation leads to confusion #1
>>>
>>> Starting point:
>>>
>>> $ qemu-system-x86_64 -chardev udp,id=chr0,port=12345,localaddr=localhost
>>>
>>> Works. host defaults to localhost, localport defaults to 0, same as in
>>> git master.
>>>
>>> Now "forget" to specify @port:
>>>
>>> $ qemu-system-x86_64 -chardev udp,id=chr0,localaddr=localhost
>>> qemu-system-x86_64: -chardev udp,id=chr0,localaddr=localhost: Parameter 'backend.data.remote.data.port' is missing
>>>
>>> Again, the visitor's error message uses "standard" notation.
>>
>> The output isn't wrong, it's just more verbose than necessary.
>>
>> Getting this one shortened is a bit harder because the right name is
>> ambiguous, the user didn't specify anything we can just print back.
>>
>> Possibly in CLI context, making use of any wildcard aliases would be a
>> reasonable strategy. This would reduce this one to just 'port'.
>
> Which of the possible keys we use in the error message boils down to
> picking a preferred key.
>
>>> We obediently do what the error message tells us to do:
>>>
>>> $ qemu-system-x86_64 -chardev udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345
>>> qemu-system-x86_64: -chardev udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345: Parameters 'backend.*' used inconsistently
>>>
>>> Mission accomplished: confusion :)
>>
>> This one already fails before aliases do their work. The reason is that
>> the default key for -chardev is 'backend', and QMP and CLI use the same
>> name 'backend' for two completely different things.
>
> Right. I was confused (and the mission was truly accomplished).
>
>> We could rename the default key into 'x-backend' and make it behave the
>> same as 'backend', then the keyval parser would only fail when you
>> explicitly wrote '-chardev backend=udp,...' and the problem is more
>> obvious.
>
> Technically a compatibility break, but we can hope that the longhand
> form we change isn't used.
>
>> By the way, we have a very similar issue with '-drive file=...', if we
>> ever want to parse that one with the keyval parser. It can be both a
>> string for the filename or an object for the configuration of the 'file'
>> child that many block drivers have.
>
> Should I be running for the hills?
>
>>> Example: manual transformation leads to confusion #2
>>>
>>> Confusion is possible even without tricking the user into mixing
>>> "standard" and "alternate" explicitly:
>>>
>>> $ qemu-system-x86_64 -chardev udp,id=chr0,backend.data.remote.type=udp
>>> qemu-system-x86_64: -chardev udp,id=chr0,backend.data.remote.type=udp: Parameters 'backend.*' used inconsistently
>>>
>>> Here, the user tried to stick to "standard", but forgot to specify a
>>> required member. The transformation machinery then "helpfully"
>>> transformed nothing into something, which made the visitor throw up.
>>
>> Not the visitor, but the keyval parser. Same problem as above.
>>
>>> Clear error reporting is a critical part of a human-friendly interface.
>>> We have two separate problems with it:
>>>
>>> 1. The visitor reports errors as if aliases didn't exist
>>>
>>> Fixing this is "merely" a matter of tracing back alias applications.
>>> More complexity...
>>>
>>> 2. The visitor reports errors as if manual transformation didn't exist
>>>
>>> Manual transformation can distort the users input beyond recognition.
>>> The visitor reports errors for the transformed input.
>>>
>>> To fix this one, we'd have to augment the parse tree so it points
>>> back at the actual user input, and then make the manual
>>> transformations preserve that. Uff!
>>
>> Manual transformations are hard to write in a way that they give perfect
>> results. As long as they are only used for legacy syntax and we expect
>> users to move to a new way to spell things, this might be acceptable for
>> a transition period until we remove the old syntax.
>
> Valid point.
>
>> In other cases, the easiest way is probably to duplicate even more of
>> the schema and manually make sure that the visitor will accept the input
>> before we transform it.
>>
>> The best way to avoid this is providing tools in QAPI that make manual
>> transformations unnecessary.
>
> Reducing the amount of handwritten code is good, as long as the price is
> reasonable. Complex code fetches a higher price.
>
> I think there are a couple of ways to skin this cat.
>
> 0. Common to all ways: specify "standard" in the QAPI schema. This
> specifies both the "standard" wire format, its parse tree
> (represented as QObject), and the "standard" C interface (C types,
> basically).
>
> Generic parsers parse into the parse tree. The appropriate input
> visitor validates and converts to C types.
>
> 1. Parse "alternate" by any means (QemuOpts, keyval, ad hoc), validate,
> then transform for the "standard" C interface. Parsing and
> validating can fail, the transformation can't.
>
> Drawbacks:
>
> * We duplicate validation, which is a standing invitation for bugs.
>
> * Longwinded, handwritten transformation code. Less vulnerable to
> bugs than validation code, I believe.
>
> 2. Parse "alternate" by any means (QemuOpts, keyval, ad hoc), transform
> to "standard" parse tree.
>
> Drawbacks:
>
> * Bad error messages from input visitor.
>
> * The handwritten transformation code is more longwinded than with 1.
>
> 3. Specify "alternate" in the QAPI schema. Map "alternate" C interface
> to the "standard" one.
>
> Drawbacks:
>
> * QAPI schema code duplication
>
> * Longwinded, handwritten mapping code.
>
> This is what we did with SocketAddressLegacy.
>
> 4. Extend the QAPI schema language to let us specify non-standard wire
> format(s). Use that to get the "alternate" wire format we need.
>
> Drawbacks:
>
> * QAPI schema language and generator become more complex.
>
> * Input visitor(s) become more complex.
>
> * Unless we accept "alternate" everywhere, we need a way to limit it
> to where it's actually needed. More complexity.
>
> The concrete proposal on the table is aliases. They are not a
> complete solution. To solve the whole problem, we combine them with
> 2. We accept 4's drawbacks for a (sizable) reduction of 2's.
> Additionally, we accept "ghost" aliases.
>
> 5. Extend the QAPI schema language to let us specify "alternate" wire
> formats for "standard" types
>
> This is like 3, except the schema code is mostly referenced instead
> duplicated, and the mapping code is generated. Something like
> "derive type T' from T with these members moved / renamed".
>
> Drawbacks
>
> * QAPI schema language and generator become more complex.
>
> * Unlike 4, we don't have working code.
>
> Like 4, this will likely solve only part of the problem.
I got one more:
6. Stir more sugar into the input visitor we use with keyval_parse():
- Recognze unique suffix of full key. Example: "ipv6" as
abbreviation of "backend.data.remote.data.ipv4".
- Default union type tag when variant members of exactly one branch
are present. Example: when we got "backend.data.addr.data.host",
"backend.data.addr.type" defaults to "inet".
Beware, this is even hairier than it may look. For instance, we want
to expand "host=playground.redhat.com,port=12345" into
backend.type=socket
backend.data.addr.type=inet
backend.data.addr.data.host=playground.redhat.com
backend.data.addr.data.port=12345
and not into
backend.type=udp
backend.data.remote.type=inet
backend.data.remote.data.host=playground.redhat.com
backend.data.remote.data.port=12345
I'm afraid this idea also solves only part of the problem.
> Which one is the least bad?
>
> If this was just about dragging deprecated interfaces to the end of
> their grace period, I'd say "whatever is the least work", and that's
> almost certainly whatever we have now, possibly hacked up to use the
> appropriate internal interface.
>
> Unfortunately, it is also about providing a friendlier interface to
> humans "forever".
>
> With 4 or 5, we invest into infrastructure once, then maintain it
> forever, to make solving the problem easier and the result easier to
> maintain.
>
> Whether the investment will pay off depends on how big and bad the
> problem is. Hard to say. One of the reasons we're looking at -chardev
> now is that we believe it's the worst of the bunch. But how big is the
> bunch, and how bad are the others in there?
>
>>> I'm afraid I need another round of thinking on how to best drag legacy
>>> syntax along when we QAPIfy.
>>
>> Let me know when you've come to any conclusions (or more things to
>> discuss).
>
> Is 3 too awful to contemplate?
Am 13.10.2021 um 13:10 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
>
> > Kevin Wolf <kwolf@redhat.com> writes:
> >
> >> Am 11.10.2021 um 09:44 hat Markus Armbruster geschrieben:
> >>> Kevin Wolf <kwolf@redhat.com> writes:
> >>>
> >>> [...]
> >>>
> >>> > What I had in mind was using the schema to generate the necessary code,
> >>> > using the generic keyval parser everywhere, and just providing a hook
> >>> > where the QDict could be modified after the keyval parser and before the
> >>> > visitor. Most command line options would not have any explicit code for
> >>> > parsing input, only the declarative schema and the final option handler
> >>> > getting a QAPI type (which could actually be the corresponding QMP
> >>> > command handler in the common case).
> >>>
> >>> A walk to the bakery made me see a problem with transforming the qdict
> >>> between keyval parsing and the visitor: error reporting. On closer
> >>> investigation, the problem exists even with just aliases.
> >>
> >> I already commented on part of this on IRC, but let me reply here as
> >> well.
> >>
> >> On general remark is that I would make the same distinction between
> >> aliases for compatibility and usability that I mentioned elsewhere in
> >> this thread.
> >>
> >> In the case of compatibility, it's already a concession that we still
> >> accept the option - suboptimal error messages for incorrect command
> >> lines aren't a major concern for me there. Users are supposed to move to
> >> the new syntax anyway.
> >
> > Well, these aren't "incorrect", merely deprecated. Bad UX is still bad
> > there, but at least it'll "fix" itself when the deprecated part goes
> > away.
Most of the error messages aren't "incorrect" either, merely suboptimal
and guiding the user towards verbose non-deprecated alternatives.
> >>> We obediently do what the error message tells us to do:
> >>>
> >>> $ qemu-system-x86_64 -chardev udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345
> >>> qemu-system-x86_64: -chardev udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345: Parameters 'backend.*' used inconsistently
> >>>
> >>> Mission accomplished: confusion :)
> >>
> >> This one already fails before aliases do their work. The reason is that
> >> the default key for -chardev is 'backend', and QMP and CLI use the same
> >> name 'backend' for two completely different things.
> >
> > Right. I was confused (and the mission was truly accomplished).
> >
> >> We could rename the default key into 'x-backend' and make it behave the
> >> same as 'backend', then the keyval parser would only fail when you
> >> explicitly wrote '-chardev backend=udp,...' and the problem is more
> >> obvious.
> >
> > Technically a compatibility break, but we can hope that the longhand
> > form we change isn't used.
No, it's not a compatibility break. Existing command lines can only have
'backend=...', but not 'backend.*=...', so there is no conflict and they
keep working.
> >> By the way, we have a very similar issue with '-drive file=...', if we
> >> ever want to parse that one with the keyval parser. It can be both a
> >> string for the filename or an object for the configuration of the 'file'
> >> child that many block drivers have.
> >
> > Should I be running for the hills?
If that means that I can then just commit whatever feels right to me? :-P
> >>> Example: manual transformation leads to confusion #2
> >>>
> >>> Confusion is possible even without tricking the user into mixing
> >>> "standard" and "alternate" explicitly:
> >>>
> >>> $ qemu-system-x86_64 -chardev udp,id=chr0,backend.data.remote.type=udp
> >>> qemu-system-x86_64: -chardev udp,id=chr0,backend.data.remote.type=udp: Parameters 'backend.*' used inconsistently
> >>>
> >>> Here, the user tried to stick to "standard", but forgot to specify a
> >>> required member. The transformation machinery then "helpfully"
> >>> transformed nothing into something, which made the visitor throw up.
> >>
> >> Not the visitor, but the keyval parser. Same problem as above.
> >>
> >>> Clear error reporting is a critical part of a human-friendly interface.
> >>> We have two separate problems with it:
> >>>
> >>> 1. The visitor reports errors as if aliases didn't exist
> >>>
> >>> Fixing this is "merely" a matter of tracing back alias applications.
> >>> More complexity...
> >>>
> >>> 2. The visitor reports errors as if manual transformation didn't exist
> >>>
> >>> Manual transformation can distort the users input beyond recognition.
> >>> The visitor reports errors for the transformed input.
> >>>
> >>> To fix this one, we'd have to augment the parse tree so it points
> >>> back at the actual user input, and then make the manual
> >>> transformations preserve that. Uff!
> >>
> >> Manual transformations are hard to write in a way that they give perfect
> >> results. As long as they are only used for legacy syntax and we expect
> >> users to move to a new way to spell things, this might be acceptable for
> >> a transition period until we remove the old syntax.
> >
> > Valid point.
> >
> >> In other cases, the easiest way is probably to duplicate even more of
> >> the schema and manually make sure that the visitor will accept the input
> >> before we transform it.
> >>
> >> The best way to avoid this is providing tools in QAPI that make manual
> >> transformations unnecessary.
> >
> > Reducing the amount of handwritten code is good, as long as the price is
> > reasonable. Complex code fetches a higher price.
> >
> > I think there are a couple of ways to skin this cat.
> >
> > 0. Common to all ways: specify "standard" in the QAPI schema. This
> > specifies both the "standard" wire format, its parse tree
> > (represented as QObject), and the "standard" C interface (C types,
> > basically).
> >
> > Generic parsers parse into the parse tree. The appropriate input
> > visitor validates and converts to C types.
> >
> > 1. Parse "alternate" by any means (QemuOpts, keyval, ad hoc), validate,
> > then transform for the "standard" C interface. Parsing and
> > validating can fail, the transformation can't.
> >
> > Drawbacks:
> >
> > * We duplicate validation, which is a standing invitation for bugs.
> >
> > * Longwinded, handwritten transformation code. Less vulnerable to
> > bugs than validation code, I believe.
> >
> > 2. Parse "alternate" by any means (QemuOpts, keyval, ad hoc), transform
> > to "standard" parse tree.
> >
> > Drawbacks:
> >
> > * Bad error messages from input visitor.
> >
> > * The handwritten transformation code is more longwinded than with 1.
> >
> > 3. Specify "alternate" in the QAPI schema. Map "alternate" C interface
> > to the "standard" one.
> >
> > Drawbacks:
> >
> > * QAPI schema code duplication
> >
> > * Longwinded, handwritten mapping code.
> >
> > This is what we did with SocketAddressLegacy.
> >
> > 4. Extend the QAPI schema language to let us specify non-standard wire
> > format(s). Use that to get the "alternate" wire format we need.
> >
> > Drawbacks:
> >
> > * QAPI schema language and generator become more complex.
> >
> > * Input visitor(s) become more complex.
> >
> > * Unless we accept "alternate" everywhere, we need a way to limit it
> > to where it's actually needed. More complexity.
> >
> > The concrete proposal on the table is aliases. They are not a
> > complete solution. To solve the whole problem, we combine them with
> > 2. We accept 4's drawbacks for a (sizable) reduction of 2's.
> > Additionally, we accept "ghost" aliases.
We combine it with 2 to solve these problems:
* Automatically determining the union type for SocketAddressLegacy
* Accepting short-form booleans (deprecated since 6.0, can be dropped)
* Diverging default values between CLI and QMP.
This includes a case in chardev-udp where QMP requires a whole
SocketAddressLegacy or nothing, but CLI accepts specifying only one of
host/port and provides a default for the other one.
* Enable aliases for chardev types (= aliases for enum values)
Solving these in generic QAPI code would probably be possible, but apart
from the short-form booleans the drawbacks of 2 are pretty much
insignificant (especially the error messages part doesn't apply), so it
feels tolerable.
> > 5. Extend the QAPI schema language to let us specify "alternate" wire
> > formats for "standard" types
> >
> > This is like 3, except the schema code is mostly referenced instead
> > duplicated, and the mapping code is generated. Something like
> > "derive type T' from T with these members moved / renamed".
> >
> > Drawbacks
> >
> > * QAPI schema language and generator become more complex.
> >
> > * Unlike 4, we don't have working code.
> >
> > Like 4, this will likely solve only part of the problem.
>
> I got one more:
>
> 6. Stir more sugar into the input visitor we use with keyval_parse():
>
> - Recognze unique suffix of full key. Example: "ipv6" as
> abbreviation of "backend.data.remote.data.ipv4".
Deciding "unique" in the visitor code feels tricky. You don't know what
future code will visit.
The only option I see is that the QAPI generator already compiles a full
list of possible abbreviations for every object type. (Obviously fails
for 'any', but I think this is not a problem.) Ugly.
Maintaining compatibility feels hard. Adding a new member "ipv4" to a
completely different type that might be used in a different union
branch, would make this stop working, probably without anyone noticing.
> - Default union type tag when variant members of exactly one branch
> are present. Example: when we got "backend.data.addr.data.host",
> "backend.data.addr.type" defaults to "inet".
>
> Beware, this is even hairier than it may look. For instance, we want
> to expand "host=playground.redhat.com,port=12345" into
>
> backend.type=socket
> backend.data.addr.type=inet
> backend.data.addr.data.host=playground.redhat.com
> backend.data.addr.data.port=12345
>
> and not into
>
> backend.type=udp
> backend.data.remote.type=inet
> backend.data.remote.data.host=playground.redhat.com
> backend.data.remote.data.port=12345
>
> I'm afraid this idea also solves only part of the problem.
Am I misunderstanding how you intended it to work? I thought it wouldn't
accept host=... at all because it isn't a unique suffix. In which case
it would obviously solve even less of the problem.
> > Which one is the least bad?
> >
> > If this was just about dragging deprecated interfaces to the end of
> > their grace period, I'd say "whatever is the least work", and that's
> > almost certainly whatever we have now, possibly hacked up to use the
> > appropriate internal interface.
> >
> > Unfortunately, it is also about providing a friendlier interface to
> > humans "forever".
> >
> > With 4 or 5, we invest into infrastructure once, then maintain it
> > forever, to make solving the problem easier and the result easier to
> > maintain.
> >
> > Whether the investment will pay off depends on how big and bad the
> > problem is. Hard to say. One of the reasons we're looking at -chardev
> > now is that we believe it's the worst of the bunch. But how big is the
> > bunch, and how bad are the others in there?
> >
> >>> I'm afraid I need another round of thinking on how to best drag legacy
> >>> syntax along when we QAPIfy.
> >>
> >> Let me know when you've come to any conclusions (or more things to
> >> discuss).
> >
> > Is 3 too awful to contemplate?
I don't feel it's worth my while because the result would be only
marginally better than the existing 1.
The short-term alternative is just adding JSON and leaving the rest
alone. Maybe deprecate the old syntax and just break it at some flag day
together with -object and -device. This fixes the compatibility
problems, but leaves the usability problem unaddressed, i.e. it will
result in a very human unfriendly syntax. If that is preferable to
having suboptimal error messages in legacy cases, I'm not sure. But at
least it will be a lot easier for me.
If we do want to address a wider problem, including making CLI more
human friendly (which would possibly not only apply to -chardev, but
also -blockdev and other options if it allows more than one valid syntax
on the command line), I'm willing to work on 4 or 5.
I think mandatory (to avoid ghosts) flattening and local renames can be
solved without touching the visitor like this alias series does, just by
generating different code, like:
if (v.profile == QMP):
visit_Foo(...)
else if (v.profile == CLI):
visit_Foo_members(...)
I don't see yet how to do this for non-local renames (like localaddr ->
local.data.host for charev-udp). Making the alternate syntax mandatory
to avoid ghosts also means that the solution can't be applied to
existing options like -blockdev without breaking compatibility.
Anyway, I've made two attempts at solving a wider problem (first just
flattening simple unions, and then more generically with aliases) and
both got shot down by you. For the third attempt I'd prefer very clear
requirements before I even start because I don't feel like wasting
another year.
Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 02.10.2021 um 15:33 hat Markus Armbruster geschrieben: >> I apologize for this wall of text. It's a desparate attempt to cut >> through the complexity and my confusion, and make sense of the actual >> problems we're trying to solve. >> >> So, what problems exactly are we trying to solve? > > I'll start with replying to your final question because I think it's > more helpful to start with the big picture than with details. > > So tools like libvirt want to have a single consistent interface to > configure things on startup and at runtime. We also intend to support > configuration files that should this time support all of the options and > not just a few chosen ones. Yes. > The hypothesis is that QAPIfying the command line is the correct > solution for both of these problems, i.e. all available command line > options must be present in the QAPI schema and will be processed by a > single parser shared with QMP to make sure they are consistent. Yes. This leads us to JSON option arguments and configuration files. Well-suited for management applications that already use QMP. > Adding QAPIfied versions of individual command line options are steps > towards this goal. As soon as they exist for every option, the final > conversion from an open coded getopt() loop (or in fact a hand crafted > parser in the case of vl.c) to something generated from the QAPI schema > should be reasonably easy. Yes. > You're right that adding a second JSON-based command line interface for > every option can already achieve the goal of providing a unified > external interface, at the cost of (interface and code) duplication. Is > this duplication desirable? Certainly no. Is it acceptable? You might > get different opinions on this one. We'd certainly prefer CLI options to match corresponding QMP commands exactly. Unfortunately, existing CLI options deviate from corresponding QMP commands, and existing CLI options without corresponding QMP commands may violate QMP design rules. Note: these issues pertain to human-friendly option syntax. The machine-friendly option syntax is still limited to just a few options, and it does match QMP there. > In my opinion, we should try to get rid of hand crafted parsers where > it's reasonably possible, and take advantage of the single unified > option structure that QAPI provides. -chardev specifically has a hand > crafted parser that essentially duplicates the automatically generated > QAPI visitor code, except for the nesting and some differences in option > names. We should definitely parse JSON option arguments with the QAPI machinery, and nothing more. Parsing human-friendly arguments with it is desirable, but the need for backward compatibility can make it difficult. Even where compatibility is of no concern, simply swapping concrete JSON syntax for dotted keys may result in human interfaces that are less than friendly. Are we in agreement that this is the problem at hand? > Aliases are one tool that can help bridge these differences in a generic > way with minimal effort in the individual case. They are not _necessary_ > to solve the problem; we could instead just use manually written code to > manipulate input QDicts so that QAPI visitors accept them. Even with > aliases, there are a few things left in the chardev code that are > converted this way. Aliases just greatly reduce the amount of this code > and make the conversion declarative instead. Understood. > Now a key point in the previous paragraph is that aliases add a generic > way to do this. So even if they are immediately motivated by -chardev, > it might be worth looking at other cases they could enable if you think > that -chardev alone isn't sufficient justification to have this tool. > I guess this is the point where things become a bit less clear because > people are just hand waving with vague ideas for additional uses. > > Do we need to invest more thought on these other cases? We probably do > if it makes a difference for the answer to the question whether we want > to add aliases to our toolbox. Does it? I hope we can make a case for aliases without looking beyond CLI QAPIfication. That's a wide field already, with enough opportunity to get lost in details. If we later put aliases to other uses, we might have to adapt them some. That's okay. Designing for one problem we have and understand has a much better chance of success than trying to design for all problems we might have. There are many CLI options to be QAPIfied. -chardev is one of the more thornier ones, which makes it a useful example. >> But what about the dotted keys argument? >> >> One point of view is the difference between the JSON and the dotted keys >> argument should be concrete syntax only. Fine print: there may be >> arguments dotted keys can't express, but let's ignore that here. >> >> Another point of view is that dotted keys arguments are to JSON >> arguments what HMP is to QMP: a (hopefully) human-friendly layer on top, >> where we have a certain degree of freedom to improve on the >> machine-friendly interface for human use. > > This doesn't feel unreasonable because with HMP, there is precedence. > But this is not all what we have used dotted key syntax for so far. I'm > not against making a change there if we feel it makes sense, but we > should be clear that it is a change. Our first uses of dotted keys were green fields. Sticking to QAPI/QMP's abstract syntax was simplest. > I think we all agree that -blockdev isn't human-friendly. Dotted key > syntax makes it humanly possible to use it on the command line, but > friendly is something else entirely. It is still a 1:1 mapping of QMP to > the command line, and this is how we advertised it, too. Yes. > This has some > important advantages, like we don't have a separate parser for a > human-friendly syntax, but the generic keyval parser covers it. There are two generic parts in play: the keyval parser, and the QAPI input visitor. Using identical abstract syntax both for JSON and dotted keys arguments makes use of both parts simple: pass the argument to qobject_input_visitor_new_str() to create a visitor, then visit the QAPI type with it. When abstract syntax differs, using the keyval parser is still simple, but to use the QAPI input visitor, we need separate QAPI types. Massive code duplication in the QAPI schema. To avoid the duplication, we can instead translate the parse tree produced for the dotted keys argument. Plenty of boring code. In short: yes, using the same abstract syntax for both has advantages. > HMP in contrast means separate code for every command, or translated to > the CLI for each command line option. HMP is not defined in QAPI, it's > a separate thing that just happens to call into QAPI-based QMP code in > the common case. > > Is this what you have in mind for non-JSON command line options? We should separate the idea "HMP wraps around QMP" from its implementation as handwritten, boilerplate-heavy code. All but the latest, QAPI-based CLI options work pretty much like HMP: a bit of code generation (hxtool), a mix of common and ad hoc parsers, boring handwritten code to translate from parse tree to internal C interfaces (which are often QMP command handlers), and to stitch it all together. Reducing the amount of boring handwritten code is equally desirable for HMP and CLI. So is specifying the interface in QAPI instead of older, less powerful schema languages like hxtool and QemuOpts. There are at least three problems: 1. Specifying CLI in QAPI hasn't progressed beyond the proof-of-concept stage. 2. Backward compatibility issues and doubts have defeated attempts to replace gnarly stuff, in particular QemuOpts. 3. How to best bridge abstract syntax differences has been unclear. Whether the differences are historical accidents or intentional for ease of human use doesn't matter. Aliases feel like a contribution towards solving 3. I don't think 1. is particularly difficult. It's just a big chunk of work that isn't really useful without solutions for 2. and 3. To me, 2. feels intractable head on. Perhaps we better bypass the problem by weakening the compatibility promise like we did for HMP. > What I had in mind was using the schema to generate the necessary code, > using the generic keyval parser everywhere, and just providing a hook > where the QDict could be modified after the keyval parser and before the > visitor. Most command line options would not have any explicit code for > parsing input, only the declarative schema and the final option handler > getting a QAPI type (which could actually be the corresponding QMP > command handler in the common case). > > I think these are very different ideas. If we want HMP-like, then all > the keyval based code paths we've built so far don't make much sense. I'm not sure the differences are "very" :) I think you start at "human-friendly and machine-friendly should use the abstract syntax defined in the QAPI schema, except where human-friendly has to differ, say for backward compatibility". This approach considers differences a blemish. The "standard" abstract syntax (the one defined in the QAPI schema) should be accepted in addition to the "alternate" one whenever possible, so "modern" use can avoid the blemishes, and blemishes can be removed once they have fallen out of use. "Alternate" should not be limited to human-friendly. Not limiting keeps things consistent, which is good, because differences are bad. Is that a fair description? I start at "human-friendly syntax should be as friendly as we can make it, except where we have to compromise, say for backward compatibility". This approach embraces differences. Mind, not differences just for differences sake, only where they help users. Additionally accepting the "standard" abstract syntax is useful only where it helps users. "Alternate" should be limited to human-friendly. Different approaches, without doubt. Yet both have to deal with differences, and both could use similar techniques and machinery for that. You're proposing to do the bulk of the work with aliases, and to have a tree-transforming hook for the remainder. Makes sense to me. However, in sufficiently gnarly cases, we might have to bypass all this and keep using handwritten code until the backward compatibility promise is history: see 2. above. In addition each approach has its own, special needs. Yours adds "alternate" syntax to QMP. This poses documentation and introspection problems. The introspection solutions will then necessitate management application updates. Mine trades these problems for others, namely how to generate parsers for two different syntaxes from one QAPI schema. Do I make sense? >> Both -chardev and QMP chardev-add use the same helpers (there are minor >> differences that don't matter here). The former basically translates >> its flat argument into the latter's arguments. HMP chardev-add is just >> like -chardev. >> >> The quickest way to QAPIfy existing -chardev is probably the one we >> chose for -object: if the argument is JSON, use the new, QAPI-based >> interface, else use the old interface. > > -chardev doesn't quite translate into chardev-add arguments. Converting > into the input of a QMP command normally means providing a QDict that > can be accepted by the QAPI-generated visitor code, and then using that > QAPI parser to create the C object. What -chardev does instead is using > an entirely separate parser to create the C object directly, without > going through any QAPI code. Yes, and that's quite some extra code, with plenty of opportunity for inconsistency. > After QAPIfication, both code paths go through the QAPI code and undergo > the same validations etc. > > If we still have code paths that completely bypass QAPI, it's hard to > call the command line option fully QAPIfied. Of course, if you don't > care about duplication and inconsistencies between the interfaces, > though, and just want to achieve the initially stated goal of providing > at least one interface consistent between QMP and CLI (besides others) > and a config file, then it might be QAPIfied enough. Reworking human-friendly -chardev to target QMP instead of a C interface shared with QMP would be nice. Just because prior attempts to replace gnarly stuff compatibly have failed doesn't mean this one must fail. >> Now the question that matters for this series: how can QAPI aliases >> help us with the things discussed above? > > The translation needs to be implemented somehow. The obvious way is just > writing, reviewing and maintaining code that manually translates. (We > don't have this code yet for a fully QAPIfied -chardev. We only have > code that bypasses QAPI, i.e. translates to the wrong target format.) > > Aliases help because they allow reducing the amount of code in favour of > data, the alias declarations in the schema. Understood. >> If we use the flat variation just internally, say for -chardev's dotted >> keys argument, then introspection is not needed. We'd use "with >> aliases" just for translating -chardev's dotted keys argument. > > Note that we're doing more translations that just flattening with > aliases, some options have different names in QMP and the CLI. > > But either way, yes, alias support could be optional so that you need to > explicitly enable it when creating a visitor, and then only the CLI code > could actually enable it. > > Limits the possible future uses for the new tool in our toolbox, but is > sufficient for -chardev. We can always extend support for it later > if/when we actually want to make use of it outside of the CLI. Yes. >> Valid argument. CLI with JSON argument should match QMP. Even when >> that means both are excessively nested. >> >> CLI with dotted keys is a different matter, in my opinion. > > I'm skipping quite a bit of text here, mainly because I think we need an > answer first if we really want to switch the keyval options to be > HMP-like in more than just the support status (see above). > > Obviously, the other relevant question would then be if it also ends up > like HMP in that most interesting functionality isn't even available and > you're forced to use QMP/JSON when you're serious. I figure this is because we have let "QMP first" degenerate into "QMP first, HMP never" sometimes, and we did so mostly because HMP is extra work that was hard to justify. What if you could get the 1:1 HMP command basically for free? It may not be the most human-friendly command possible. But it should be a start. > I guess I can go into more details once we have answered these > fundamental questions. > > (We could and should have talked about them and about whether you want > to have aliases at all a year ago, before going into detailed code > review and making error messages perfect in code that we might throw > away anyway...) I doubt you could possibly be more right than that!
Am 05.10.2021 um 15:49 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 02.10.2021 um 15:33 hat Markus Armbruster geschrieben: > >> I apologize for this wall of text. It's a desparate attempt to cut > >> through the complexity and my confusion, and make sense of the actual > >> problems we're trying to solve. > >> > >> So, what problems exactly are we trying to solve? > > > > I'll start with replying to your final question because I think it's > > more helpful to start with the big picture than with details. > > > > So tools like libvirt want to have a single consistent interface to > > configure things on startup and at runtime. We also intend to support > > configuration files that should this time support all of the options and > > not just a few chosen ones. > > Yes. > > > The hypothesis is that QAPIfying the command line is the correct > > solution for both of these problems, i.e. all available command line > > options must be present in the QAPI schema and will be processed by a > > single parser shared with QMP to make sure they are consistent. > > Yes. > > This leads us to JSON option arguments and configuration files. > Well-suited for management applications that already use QMP. > > > Adding QAPIfied versions of individual command line options are steps > > towards this goal. As soon as they exist for every option, the final > > conversion from an open coded getopt() loop (or in fact a hand crafted > > parser in the case of vl.c) to something generated from the QAPI schema > > should be reasonably easy. > > Yes. > > > You're right that adding a second JSON-based command line interface for > > every option can already achieve the goal of providing a unified > > external interface, at the cost of (interface and code) duplication. Is > > this duplication desirable? Certainly no. Is it acceptable? You might > > get different opinions on this one. > > We'd certainly prefer CLI options to match corresponding QMP commands > exactly. > > Unfortunately, existing CLI options deviate from corresponding QMP > commands, and existing CLI options without corresponding QMP commands > may violate QMP design rules. > > Note: these issues pertain to human-friendly option syntax. The > machine-friendly option syntax is still limited to just a few options, > and it does match QMP there. On the other hand, we only have a handful of existing options that are very complex. Most of them aren't even structured and just take a single scalar value. So I'm relatively sure that we know the big ones, and we're working on them. Another point here is that I would argue that there is a difference between existing options and human-friendly options. Not all existing options are human-friendly just because they aren't machine friendly either, and there is probably potential for human-friendly syntax that doesn't exist yet. So I would treat support for existing options (i.e. compatibility) and support for human-friendly options (i.e. usability) as two separate problems. > > In my opinion, we should try to get rid of hand crafted parsers where > > it's reasonably possible, and take advantage of the single unified > > option structure that QAPI provides. -chardev specifically has a hand > > crafted parser that essentially duplicates the automatically generated > > QAPI visitor code, except for the nesting and some differences in option > > names. > > We should definitely parse JSON option arguments with the QAPI > machinery, and nothing more. Yes, no doubt. (And we don't even consistently do that yet, like device-add going through QemuOpts after going through QAPI and throwing away all type information and silently ignoring anything it doesn't know to handle.) > Parsing human-friendly arguments with it is desirable, but the need for > backward compatibility can make it difficult. Even where compatibility > is of no concern, simply swapping concrete JSON syntax for dotted keys > may result in human interfaces that are less than friendly. > > Are we in agreement that this is the problem at hand? As far as I am concerned, compatibility is the problem at hand, usability isn't. This doesn't mean that I'm opposed to having actually human friendly options, quite the opposite. But getting machine friendly options is already a big project. Making human interfaces friendlier would be adding another project of similar size, and I don't feel like tackling a second project at the same time when the first one is already hard. > > Aliases are one tool that can help bridge these differences in a generic > > way with minimal effort in the individual case. They are not _necessary_ > > to solve the problem; we could instead just use manually written code to > > manipulate input QDicts so that QAPI visitors accept them. Even with > > aliases, there are a few things left in the chardev code that are > > converted this way. Aliases just greatly reduce the amount of this code > > and make the conversion declarative instead. > > Understood. > > > Now a key point in the previous paragraph is that aliases add a generic > > way to do this. So even if they are immediately motivated by -chardev, > > it might be worth looking at other cases they could enable if you think > > that -chardev alone isn't sufficient justification to have this tool. > > I guess this is the point where things become a bit less clear because > > people are just hand waving with vague ideas for additional uses. > > > > Do we need to invest more thought on these other cases? We probably do > > if it makes a difference for the answer to the question whether we want > > to add aliases to our toolbox. Does it? > > I hope we can make a case for aliases without looking beyond CLI > QAPIfication. That's a wide field already, with enough opportunity to > get lost in details. > > If we later put aliases to other uses, we might have to adapt them some. > That's okay. Designing for one problem we have and understand has a > much better chance of success than trying to design for all problems we > might have. > > There are many CLI options to be QAPIfied. -chardev is one of the more > thornier ones, which makes it a useful example. Good, I agree. > >> But what about the dotted keys argument? > >> > >> One point of view is the difference between the JSON and the dotted keys > >> argument should be concrete syntax only. Fine print: there may be > >> arguments dotted keys can't express, but let's ignore that here. > >> > >> Another point of view is that dotted keys arguments are to JSON > >> arguments what HMP is to QMP: a (hopefully) human-friendly layer on top, > >> where we have a certain degree of freedom to improve on the > >> machine-friendly interface for human use. > > > > This doesn't feel unreasonable because with HMP, there is precedence. > > But this is not all what we have used dotted key syntax for so far. I'm > > not against making a change there if we feel it makes sense, but we > > should be clear that it is a change. > > Our first uses of dotted keys were green fields. Sticking to QAPI/QMP's > abstract syntax was simplest. > > > I think we all agree that -blockdev isn't human-friendly. Dotted key > > syntax makes it humanly possible to use it on the command line, but > > friendly is something else entirely. It is still a 1:1 mapping of QMP to > > the command line, and this is how we advertised it, too. > > Yes. > > > This has some > > important advantages, like we don't have a separate parser for a > > human-friendly syntax, but the generic keyval parser covers it. > > There are two generic parts in play: the keyval parser, and the QAPI > input visitor. Fair point. > Using identical abstract syntax both for JSON and dotted keys arguments > makes use of both parts simple: pass the argument to > qobject_input_visitor_new_str() to create a visitor, then visit the QAPI > type with it. > > When abstract syntax differs, using the keyval parser is still simple, > but to use the QAPI input visitor, we need separate QAPI types. Massive > code duplication in the QAPI schema. > > To avoid the duplication, we can instead translate the parse tree > produced for the dotted keys argument. Plenty of boring code. > > In short: yes, using the same abstract syntax for both has advantages. Even with duplication in the schema, you still have to translate unless you want to duplicate all of the logic, too. The difference is just that instead of translating between QObjects, you would be translating between C structs. So yes, using different abstract syntax means translation, which in turn can mean plenty of boring code (hopefully - might also be buggy rather than boring) if we don't support ways to automate the conversion. > > HMP in contrast means separate code for every command, or translated to > > the CLI for each command line option. HMP is not defined in QAPI, it's > > a separate thing that just happens to call into QAPI-based QMP code in > > the common case. > > > > Is this what you have in mind for non-JSON command line options? > > We should separate the idea "HMP wraps around QMP" from its > implementation as handwritten, boilerplate-heavy code. > > All but the latest, QAPI-based CLI options work pretty much like HMP: a > bit of code generation (hxtool), a mix of common and ad hoc parsers, > boring handwritten code to translate from parse tree to internal C > interfaces (which are often QMP command handlers), and to stitch it all > together. > > Reducing the amount of boring handwritten code is equally desirable for > HMP and CLI. > > So is specifying the interface in QAPI instead of older, less powerful > schema languages like hxtool and QemuOpts. > > There are at least three problems: > > 1. Specifying CLI in QAPI hasn't progressed beyond the proof-of-concept > stage. > > 2. Backward compatibility issues and doubts have defeated attempts to > replace gnarly stuff, in particular QemuOpts. > > 3. How to best bridge abstract syntax differences has been unclear. > Whether the differences are historical accidents or intentional for ease > of human use doesn't matter. > > Aliases feel like a contribution towards solving 3. > > I don't think 1. is particularly difficult. It's just a big chunk of > work that isn't really useful without solutions for 2. and 3. > > To me, 2. feels intractable head on. Perhaps we better bypass the > problem by weakening the compatibility promise like we did for HMP. Can we define 2 in a bit more specific terms? I feel the biggest part of 2 is actually 3. You mention QemuOpts, but how commonly are the potentially problematic features of it even used? Short booleans are deprecated and could be dropped in 6.2. merge_lists may or may not have to be replicated. I know building lists from repeated keys is said to be a thing, where does it happen? I've worked on some of the big ones (blockdev, chardev, object, device) and haven't come across it yet. Can we have an intermediate state where CLI parsing involves both QAPI generated options and manually written ones? So that we can actually put the QAPIfication work already done to use instead of just having a promise that it will make things possible eventually, in a few years? > > What I had in mind was using the schema to generate the necessary code, > > using the generic keyval parser everywhere, and just providing a hook > > where the QDict could be modified after the keyval parser and before the > > visitor. Most command line options would not have any explicit code for > > parsing input, only the declarative schema and the final option handler > > getting a QAPI type (which could actually be the corresponding QMP > > command handler in the common case). > > > > I think these are very different ideas. If we want HMP-like, then all > > the keyval based code paths we've built so far don't make much sense. > > I'm not sure the differences are "very" :) > > I think you start at "human-friendly and machine-friendly should use the > abstract syntax defined in the QAPI schema, except where human-friendly > has to differ, say for backward compatibility". > > This approach considers differences a blemish. The "standard" abstract > syntax (the one defined in the QAPI schema) should be accepted in > addition to the "alternate" one whenever possible, so "modern" use can > avoid the blemishes, and blemishes can be removed once they have fallen > out of use. > > "Alternate" should not be limited to human-friendly. Not limiting keeps > things consistent, which is good, because differences are bad. > > Is that a fair description? Hm, yes and no. I do think that making the overlap between "standard" and "alternate" abstract syntax as big as possible is a good thing because it means less translation has to go on between them, and ultimately the interfaces are more consistent with each other which actually improves the human friendliness for people who get in touch with both syntaxes. The -chardev conversion mostly deals with differences that I do consider a blemish: There is no real reason why options need to have different names on both interfaces, and there is also a lot of nesting in QMP for which there is no real reason. The reason why we need to accept both is compatibility, not usability. There are also some differences that are justified by friendliness. Having "addr" as a nested struct in QMP just makes sense, and on the command line having it flattened makes more sense. So accepting "path" for "device" in ChardevHostdev is probably a case where switching both QMP and CLI to "modern" use and remove the "blemish" eventually makes sense to me. The nesting for "addr" isn't. We may even add more differences to make things human friendly. My standard for this is whether the difference serves only for compatibility (should go away eventually) or usability (should stay). Now with this, it's still open whether the "standard" syntax should only be supported in QMP or also in the CLI, and whether "alternate" syntax should only be supported in the CLI or also in QMP. Is usability actually improved by rejecting the "standard" syntax on the command line, or by rejecting "alternate" syntax in QMP? Hardly so. It's also not compatibility. So what is the justification for the difference? Maintainability? I honestly don't see the improvement there either. So I don't really see a reason for differences, but at the same time it's also not a very important question to me. If you prefer restricting things, so be it. > I start at "human-friendly syntax should be as friendly as we can make > it, except where we have to compromise, say for backward compatibility". > > This approach embraces differences. Mind, not differences just for > differences sake, only where they help users. > > Additionally accepting the "standard" abstract syntax is useful only > where it helps users. > > "Alternate" should be limited to human-friendly. I think there is a small inconsistency in this reasoning: You say that differences must help users, but then this is not the measuring stick you use in the next paragraph. If it were, you would be asking whether rejecting "standard" abstract syntax helps users, rather than whether adding it does. (Even more so because rejecting it is more work! Not much more work, but it adds a little bit of complexity.) So it seems that in practice your approach is more like "different by default, making it the same needs justification", whereas I am leaning more towards "same by default, making it different needs justification". Your idea of "human-friendly syntax should be as friendly as we can make it" isn't in conflict with either approach. The thing that the idea might actually conflict with is our time budget. > Different approaches, without doubt. Yet both have to deal with > differences, and both could use similar techniques and machinery for > that. You're proposing to do the bulk of the work with aliases, and to > have a tree-transforming hook for the remainder. Makes sense to me. > However, in sufficiently gnarly cases, we might have to bypass all this > and keep using handwritten code until the backward compatibility promise > is history: see 2. above. Possibly. I honestly can't tell how many of these cases we will have. In all of -object, we had exactly one problematic option. This could easily be handled in a tree-transforming hook. > In addition each approach has its own, special needs. > > Yours adds "alternate" syntax to QMP. This poses documentation and > introspection problems. The introspection solutions will then > necessitate management application updates. > > Mine trades these problems for others, namely how to generate parsers > for two different syntaxes from one QAPI schema. > > Do I make sense? In the long run, won't we need documentation and introspection even for things that are limited to the command line? Introspection is one of the big features enabled by CLI QAPIfication. But otherwise yes. > >> Both -chardev and QMP chardev-add use the same helpers (there are minor > >> differences that don't matter here). The former basically translates > >> its flat argument into the latter's arguments. HMP chardev-add is just > >> like -chardev. > >> > >> The quickest way to QAPIfy existing -chardev is probably the one we > >> chose for -object: if the argument is JSON, use the new, QAPI-based > >> interface, else use the old interface. > > > > -chardev doesn't quite translate into chardev-add arguments. Converting > > into the input of a QMP command normally means providing a QDict that > > can be accepted by the QAPI-generated visitor code, and then using that > > QAPI parser to create the C object. What -chardev does instead is using > > an entirely separate parser to create the C object directly, without > > going through any QAPI code. > > Yes, and that's quite some extra code, with plenty of opportunity for > inconsistency. Needless to say, opportunity that we happily made use of. > > After QAPIfication, both code paths go through the QAPI code and undergo > > the same validations etc. > > > > If we still have code paths that completely bypass QAPI, it's hard to > > call the command line option fully QAPIfied. Of course, if you don't > > care about duplication and inconsistencies between the interfaces, > > though, and just want to achieve the initially stated goal of providing > > at least one interface consistent between QMP and CLI (besides others) > > and a config file, then it might be QAPIfied enough. > > Reworking human-friendly -chardev to target QMP instead of a C interface > shared with QMP would be nice. Just because prior attempts to replace > gnarly stuff compatibly have failed doesn't mean this one must fail. I mean, for -chardev specifically, you don't even have to take a guess. The patches exist, a git tag with them is mentioned in the cover letter of this series, and they have just been waiting for about a year for their dependency (QAPI aliases) to be merged. If we don't do aliases, I'll have to rework them to do everything in code instead. It's doable, even if the chardev code wouldn't shrink as nicely as with the current patches. I just need to know whether it has to be done or not. > >> Now the question that matters for this series: how can QAPI aliases > >> help us with the things discussed above? > > > > The translation needs to be implemented somehow. The obvious way is just > > writing, reviewing and maintaining code that manually translates. (We > > don't have this code yet for a fully QAPIfied -chardev. We only have > > code that bypasses QAPI, i.e. translates to the wrong target format.) > > > > Aliases help because they allow reducing the amount of code in favour of > > data, the alias declarations in the schema. > > Understood. > > >> If we use the flat variation just internally, say for -chardev's dotted > >> keys argument, then introspection is not needed. We'd use "with > >> aliases" just for translating -chardev's dotted keys argument. > > > > Note that we're doing more translations that just flattening with > > aliases, some options have different names in QMP and the CLI. > > > > But either way, yes, alias support could be optional so that you need to > > explicitly enable it when creating a visitor, and then only the CLI code > > could actually enable it. > > > > Limits the possible future uses for the new tool in our toolbox, but is > > sufficient for -chardev. We can always extend support for it later > > if/when we actually want to make use of it outside of the CLI. > > Yes. > > >> Valid argument. CLI with JSON argument should match QMP. Even when > >> that means both are excessively nested. > >> > >> CLI with dotted keys is a different matter, in my opinion. > > > > I'm skipping quite a bit of text here, mainly because I think we need an > > answer first if we really want to switch the keyval options to be > > HMP-like in more than just the support status (see above). > > > > Obviously, the other relevant question would then be if it also ends up > > like HMP in that most interesting functionality isn't even available and > > you're forced to use QMP/JSON when you're serious. > > I figure this is because we have let "QMP first" degenerate into "QMP > first, HMP never" sometimes, and we did so mostly because HMP is extra > work that was hard to justify. > > What if you could get the 1:1 HMP command basically for free? It may > not be the most human-friendly command possible. But it should be a > start. How would you do this? The obvious way is mapping QMP 1:1 like we do for some of the command line options. But you just argued that this is not what you would prefer for the command line, so it's probably not what you have in mind for HMP either? > > I guess I can go into more details once we have answered these > > fundamental questions. > > > > (We could and should have talked about them and about whether you want > > to have aliases at all a year ago, before going into detailed code > > review and making error messages perfect in code that we might throw > > away anyway...) > > I doubt you could possibly be more right than that! So what questions need to be answered before we can make a decision? You talked a lot about how you like QMP to be different from the command line. So is restricting aliases to the command line enough to make them acceptable? Or do we need larger changes? Should I just throw them away and write translation code for -chardev manually? Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 05.10.2021 um 15:49 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 02.10.2021 um 15:33 hat Markus Armbruster geschrieben: >> >> I apologize for this wall of text. It's a desparate attempt to cut >> >> through the complexity and my confusion, and make sense of the actual >> >> problems we're trying to solve. >> >> >> >> So, what problems exactly are we trying to solve? >> > >> > I'll start with replying to your final question because I think it's >> > more helpful to start with the big picture than with details. >> > >> > So tools like libvirt want to have a single consistent interface to >> > configure things on startup and at runtime. We also intend to support >> > configuration files that should this time support all of the options and >> > not just a few chosen ones. >> >> Yes. >> >> > The hypothesis is that QAPIfying the command line is the correct >> > solution for both of these problems, i.e. all available command line >> > options must be present in the QAPI schema and will be processed by a >> > single parser shared with QMP to make sure they are consistent. >> >> Yes. >> >> This leads us to JSON option arguments and configuration files. >> Well-suited for management applications that already use QMP. >> >> > Adding QAPIfied versions of individual command line options are steps >> > towards this goal. As soon as they exist for every option, the final >> > conversion from an open coded getopt() loop (or in fact a hand crafted >> > parser in the case of vl.c) to something generated from the QAPI schema >> > should be reasonably easy. >> >> Yes. >> >> > You're right that adding a second JSON-based command line interface for >> > every option can already achieve the goal of providing a unified >> > external interface, at the cost of (interface and code) duplication. Is >> > this duplication desirable? Certainly no. Is it acceptable? You might >> > get different opinions on this one. >> >> We'd certainly prefer CLI options to match corresponding QMP commands >> exactly. >> >> Unfortunately, existing CLI options deviate from corresponding QMP >> commands, and existing CLI options without corresponding QMP commands >> may violate QMP design rules. >> >> Note: these issues pertain to human-friendly option syntax. The >> machine-friendly option syntax is still limited to just a few options, >> and it does match QMP there. > > On the other hand, we only have a handful of existing options that are > very complex. Most of them aren't even structured and just take a single > scalar value. So I'm relatively sure that we know the big ones, and > we're working on them. > > Another point here is that I would argue that there is a difference > between existing options and human-friendly options. Not all existing > options are human-friendly just because they aren't machine friendly > either, and there is probably potential for human-friendly syntax that > doesn't exist yet. > > So I would treat support for existing options (i.e. compatibility) and > support for human-friendly options (i.e. usability) as two separate > problems. That's fair. Instead of "human-friendly option syntax", I could've written "our traditional option syntax (which is better suited to humans than to machines, although it still fails to be truly friendly to anyone)", but that's a tad long, isn't it? >> > In my opinion, we should try to get rid of hand crafted parsers where >> > it's reasonably possible, and take advantage of the single unified >> > option structure that QAPI provides. -chardev specifically has a hand >> > crafted parser that essentially duplicates the automatically generated >> > QAPI visitor code, except for the nesting and some differences in option >> > names. >> >> We should definitely parse JSON option arguments with the QAPI >> machinery, and nothing more. > > Yes, no doubt. (And we don't even consistently do that yet, like > device-add going through QemuOpts after going through QAPI and throwing > away all type information and silently ignoring anything it doesn't know > to handle.) At least device-add is the last remaining user of 'gen': false. The 'any' type also leaves type checking to handwritten code, but at least it doesn't throw away type information first. Used by qom-get and qom-set. >> Parsing human-friendly arguments with it is desirable, but the need for >> backward compatibility can make it difficult. Even where compatibility >> is of no concern, simply swapping concrete JSON syntax for dotted keys >> may result in human interfaces that are less than friendly. >> >> Are we in agreement that this is the problem at hand? > > As far as I am concerned, compatibility is the problem at hand, > usability isn't. > > This doesn't mean that I'm opposed to having actually human friendly > options, quite the opposite. But getting machine friendly options is > already a big project. Making human interfaces friendlier would be > adding another project of similar size, and I don't feel like tackling > a second project at the same time when the first one is already hard. Fair enough. >> > Aliases are one tool that can help bridge these differences in a generic >> > way with minimal effort in the individual case. They are not _necessary_ >> > to solve the problem; we could instead just use manually written code to >> > manipulate input QDicts so that QAPI visitors accept them. Even with >> > aliases, there are a few things left in the chardev code that are >> > converted this way. Aliases just greatly reduce the amount of this code >> > and make the conversion declarative instead. >> >> Understood. >> >> > Now a key point in the previous paragraph is that aliases add a generic >> > way to do this. So even if they are immediately motivated by -chardev, >> > it might be worth looking at other cases they could enable if you think >> > that -chardev alone isn't sufficient justification to have this tool. >> > I guess this is the point where things become a bit less clear because >> > people are just hand waving with vague ideas for additional uses. >> > >> > Do we need to invest more thought on these other cases? We probably do >> > if it makes a difference for the answer to the question whether we want >> > to add aliases to our toolbox. Does it? >> >> I hope we can make a case for aliases without looking beyond CLI >> QAPIfication. That's a wide field already, with enough opportunity to >> get lost in details. >> >> If we later put aliases to other uses, we might have to adapt them some. >> That's okay. Designing for one problem we have and understand has a >> much better chance of success than trying to design for all problems we >> might have. >> >> There are many CLI options to be QAPIfied. -chardev is one of the more >> thornier ones, which makes it a useful example. > > Good, I agree. > >> >> But what about the dotted keys argument? >> >> >> >> One point of view is the difference between the JSON and the dotted keys >> >> argument should be concrete syntax only. Fine print: there may be >> >> arguments dotted keys can't express, but let's ignore that here. >> >> >> >> Another point of view is that dotted keys arguments are to JSON >> >> arguments what HMP is to QMP: a (hopefully) human-friendly layer on top, >> >> where we have a certain degree of freedom to improve on the >> >> machine-friendly interface for human use. >> > >> > This doesn't feel unreasonable because with HMP, there is precedence. >> > But this is not all what we have used dotted key syntax for so far. I'm >> > not against making a change there if we feel it makes sense, but we >> > should be clear that it is a change. >> >> Our first uses of dotted keys were green fields. Sticking to QAPI/QMP's >> abstract syntax was simplest. >> >> > I think we all agree that -blockdev isn't human-friendly. Dotted key >> > syntax makes it humanly possible to use it on the command line, but >> > friendly is something else entirely. It is still a 1:1 mapping of QMP to >> > the command line, and this is how we advertised it, too. >> >> Yes. >> >> > This has some >> > important advantages, like we don't have a separate parser for a >> > human-friendly syntax, but the generic keyval parser covers it. >> >> There are two generic parts in play: the keyval parser, and the QAPI >> input visitor. > > Fair point. > >> Using identical abstract syntax both for JSON and dotted keys arguments >> makes use of both parts simple: pass the argument to >> qobject_input_visitor_new_str() to create a visitor, then visit the QAPI >> type with it. >> >> When abstract syntax differs, using the keyval parser is still simple, >> but to use the QAPI input visitor, we need separate QAPI types. Massive >> code duplication in the QAPI schema. >> >> To avoid the duplication, we can instead translate the parse tree >> produced for the dotted keys argument. Plenty of boring code. >> >> In short: yes, using the same abstract syntax for both has advantages. > > Even with duplication in the schema, you still have to translate unless > you want to duplicate all of the logic, too. The difference is just that > instead of translating between QObjects, you would be translating > between C structs. True. > So yes, using different abstract syntax means translation, which in turn > can mean plenty of boring code (hopefully - might also be buggy rather > than boring) if we don't support ways to automate the conversion. We did it manually for SocketAddress / SocketAddressLegacy. Worked out okay. We don't want to do it manually for chardev-add's argument type, because the amount of translation / duplication is off-putting. >> > HMP in contrast means separate code for every command, or translated to >> > the CLI for each command line option. HMP is not defined in QAPI, it's >> > a separate thing that just happens to call into QAPI-based QMP code in >> > the common case. >> > >> > Is this what you have in mind for non-JSON command line options? >> >> We should separate the idea "HMP wraps around QMP" from its >> implementation as handwritten, boilerplate-heavy code. >> >> All but the latest, QAPI-based CLI options work pretty much like HMP: a >> bit of code generation (hxtool), a mix of common and ad hoc parsers, >> boring handwritten code to translate from parse tree to internal C >> interfaces (which are often QMP command handlers), and to stitch it all >> together. >> >> Reducing the amount of boring handwritten code is equally desirable for >> HMP and CLI. >> >> So is specifying the interface in QAPI instead of older, less powerful >> schema languages like hxtool and QemuOpts. >> >> There are at least three problems: >> >> 1. Specifying CLI in QAPI hasn't progressed beyond the proof-of-concept >> stage. >> >> 2. Backward compatibility issues and doubts have defeated attempts to >> replace gnarly stuff, in particular QemuOpts. >> >> 3. How to best bridge abstract syntax differences has been unclear. >> Whether the differences are historical accidents or intentional for ease >> of human use doesn't matter. >> >> Aliases feel like a contribution towards solving 3. >> >> I don't think 1. is particularly difficult. It's just a big chunk of >> work that isn't really useful without solutions for 2. and 3. >> >> To me, 2. feels intractable head on. Perhaps we better bypass the >> problem by weakening the compatibility promise like we did for HMP. > > Can we define 2 in a bit more specific terms? I feel the biggest part > of 2 is actually 3. > > You mention QemuOpts, but how commonly are the potentially problematic > features of it even used? Short booleans are deprecated and could be > dropped in 6.2. merge_lists may or may not have to be replicated. > I know building lists from repeated keys is said to be a thing, where > does it happen? I've worked on some of the big ones (blockdev, chardev, > object, device) and haven't come across it yet. Alright, I'll look for you :) Check out commit 2d6dcbf93fb, 396f935a9af, 54cb65d8588, f1672e6f2b6. These are fairly recent, which is kind of depressing. There may be more. When I wrote 2., I was specifically thinking of -object, where we decided not to mess the human-friendly syntax in part because we didn't fully understand what a conversion from QemuOpts to keyval could break. > Can we have an intermediate state where CLI parsing involves both QAPI > generated options and manually written ones? So that we can actually put > the QAPIfication work already done to use instead of just having a > promise that it will make things possible eventually, in a few years? We kind of sort of have that already: a few option arguments are QAPI-based. Perhaps it's time to crack 1., but in a way that accommodates handwritten options. Could be like "this option takes a string argument, and the function to process the option is not to be generated, only called (just like 'gen': false for commands). We may want to make introspection describe the argument as "unknown", to distinguish it from genuine string arguments. >> > What I had in mind was using the schema to generate the necessary code, >> > using the generic keyval parser everywhere, and just providing a hook >> > where the QDict could be modified after the keyval parser and before the >> > visitor. Most command line options would not have any explicit code for >> > parsing input, only the declarative schema and the final option handler >> > getting a QAPI type (which could actually be the corresponding QMP >> > command handler in the common case). >> > >> > I think these are very different ideas. If we want HMP-like, then all >> > the keyval based code paths we've built so far don't make much sense. >> >> I'm not sure the differences are "very" :) >> >> I think you start at "human-friendly and machine-friendly should use the >> abstract syntax defined in the QAPI schema, except where human-friendly >> has to differ, say for backward compatibility". >> >> This approach considers differences a blemish. The "standard" abstract >> syntax (the one defined in the QAPI schema) should be accepted in >> addition to the "alternate" one whenever possible, so "modern" use can >> avoid the blemishes, and blemishes can be removed once they have fallen >> out of use. >> >> "Alternate" should not be limited to human-friendly. Not limiting keeps >> things consistent, which is good, because differences are bad. >> >> Is that a fair description? > > Hm, yes and no. > > > I do think that making the overlap between "standard" and "alternate" > abstract syntax as big as possible is a good thing because it means less > translation has to go on between them, and ultimately the interfaces are > more consistent with each other which actually improves the human > friendliness for people who get in touch with both syntaxes. > > The -chardev conversion mostly deals with differences that I do consider > a blemish: There is no real reason why options need to have different > names on both interfaces, and there is also a lot of nesting in QMP for > which there is no real reason. > > The reason why we need to accept both is compatibility, not usability. Compatibility requires us to accept both, but each in its place. It doesn't actually require us to accept both in either place. > There are also some differences that are justified by friendliness. > Having "addr" as a nested struct in QMP just makes sense, and on the > command line having it flattened makes more sense. > > So accepting "path" for "device" in ChardevHostdev is probably a case > where switching both QMP and CLI to "modern" use and remove the > "blemish" eventually makes sense to me. The nesting for "addr" isn't. > > We may even add more differences to make things human friendly. My > standard for this is whether the difference serves only for > compatibility (should go away eventually) or usability (should stay). In theory, cleaning up QMP is nice. In practice, we don't have a good way to rename or move members. When we do, we have to make both old and new optional, even when you actually have to specify exactly one. This isn't the end of the world, but it is trading one kind of blemish for another. Improving on that would involve a non-trivial extension to introspection. Pain right away, gain only when all management applications that matter grok it. I think the realistic assumption is that QMP blemishes stay. Of course, we'd prefer not to copy existing QMP blemishes into new interfaces. That's why we have SocketAddress for new code, and SocketAddressLegacy for old code. Workable for simple utility types. I don't have a good solution for "bigger" types. > Now with this, it's still open whether the "standard" syntax should only > be supported in QMP or also in the CLI, and whether "alternate" syntax > should only be supported in the CLI or also in QMP. > > Is usability actually improved by rejecting the "standard" syntax on the > command line, or by rejecting "alternate" syntax in QMP? Hardly so. It's > also not compatibility. So what is the justification for the difference? > Maintainability? I honestly don't see the improvement there either. > > So I don't really see a reason for differences, but at the same time > it's also not a very important question to me. If you prefer restricting > things, so be it. I dislike adding "alternate" to machine-friendly QMP / CLI poses, because: * Adding "alternate" poses documentation and introspection problems. The introspection solutions will then necessitate management application updates. Anything we do becomes ABI immediately, so we better get it right on the first try. * Ignoring the documentation problems is a bad idea, because undocumented interfaces are. * If we ignore the introspection problems, then machines are well advised to stick to introspectable "standard". Why add "alternate" then? Complexity for nothing. * Adding "alternate" to existing interfaces is pretty much pointless. To use "alternate", machines first have to check whether this QEMU provides it, and if not, fall back to "standard". Just use "standard" and call it a day. * For new interfaces, the differences between "standard" and "alternate" are hopefully smaller than for old interfaces. Machines will likely not care for improvements "alternate" may provide over "standard". I haven't made up my mind on adding "standard" to human-friendly CLI. Too many ways to do the same thing are bound to confuse. We could end up with the standard way, the alternate way we provide for backward compatibility, the alternate way we provide for usability, and any combination thereof. Each of the three ways has its uses, though. Combinations not so much. Adding "standard" to human-friendly CLI only poses documentation problems (there is no introspection). If we limit the backward compatibility promise to machine-friendly, getting things wrong is no big deal. >> I start at "human-friendly syntax should be as friendly as we can make >> it, except where we have to compromise, say for backward compatibility". >> >> This approach embraces differences. Mind, not differences just for >> differences sake, only where they help users. >> >> Additionally accepting the "standard" abstract syntax is useful only >> where it helps users. >> >> "Alternate" should be limited to human-friendly. > > I think there is a small inconsistency in this reasoning: You say that > differences must help users, but then this is not the measuring stick > you use in the next paragraph. If it were, you would be asking whether > rejecting "standard" abstract syntax helps users, rather than whether > adding it does. (Even more so because rejecting it is more work! Not > much more work, but it adds a little bit of complexity.) Additionally accepting "standard" complicates the interface and its documentation. Whether this helps users more than it hurts them is not obvious. > So it seems that in practice your approach is more like "different by > default, making it the same needs justification", whereas I am leaning > more towards "same by default, making it different needs justification". > > Your idea of "human-friendly syntax should be as friendly as we can make > it" isn't in conflict with either approach. The thing that the idea > might actually conflict with is our time budget. Reasonable people often have quite different ideas on "human-friendly". >> Different approaches, without doubt. Yet both have to deal with >> differences, and both could use similar techniques and machinery for >> that. You're proposing to do the bulk of the work with aliases, and to >> have a tree-transforming hook for the remainder. Makes sense to me. >> However, in sufficiently gnarly cases, we might have to bypass all this >> and keep using handwritten code until the backward compatibility promise >> is history: see 2. above. > > Possibly. I honestly can't tell how many of these cases we will have. > In all of -object, we had exactly one problematic option. This could > easily be handled in a tree-transforming hook. > >> In addition each approach has its own, special needs. >> >> Yours adds "alternate" syntax to QMP. This poses documentation and >> introspection problems. The introspection solutions will then >> necessitate management application updates. >> >> Mine trades these problems for others, namely how to generate parsers >> for two different syntaxes from one QAPI schema. >> >> Do I make sense? > > In the long run, won't we need documentation and introspection even for > things that are limited to the command line? Introspection is one of the > big features enabled by CLI QAPIfication. CLI introspection is about the machine-friendly syntax, not the human-friendly one, just like monitor introspection is about QMP, not HMP. > But otherwise yes. > >> >> Both -chardev and QMP chardev-add use the same helpers (there are minor >> >> differences that don't matter here). The former basically translates >> >> its flat argument into the latter's arguments. HMP chardev-add is just >> >> like -chardev. >> >> >> >> The quickest way to QAPIfy existing -chardev is probably the one we >> >> chose for -object: if the argument is JSON, use the new, QAPI-based >> >> interface, else use the old interface. >> > >> > -chardev doesn't quite translate into chardev-add arguments. Converting >> > into the input of a QMP command normally means providing a QDict that >> > can be accepted by the QAPI-generated visitor code, and then using that >> > QAPI parser to create the C object. What -chardev does instead is using >> > an entirely separate parser to create the C object directly, without >> > going through any QAPI code. >> >> Yes, and that's quite some extra code, with plenty of opportunity for >> inconsistency. > > Needless to say, opportunity that we happily made use of. Hah! >> > After QAPIfication, both code paths go through the QAPI code and undergo >> > the same validations etc. >> > >> > If we still have code paths that completely bypass QAPI, it's hard to >> > call the command line option fully QAPIfied. Of course, if you don't >> > care about duplication and inconsistencies between the interfaces, >> > though, and just want to achieve the initially stated goal of providing >> > at least one interface consistent between QMP and CLI (besides others) >> > and a config file, then it might be QAPIfied enough. >> >> Reworking human-friendly -chardev to target QMP instead of a C interface >> shared with QMP would be nice. Just because prior attempts to replace >> gnarly stuff compatibly have failed doesn't mean this one must fail. > > I mean, for -chardev specifically, you don't even have to take a guess. > The patches exist, a git tag with them is mentioned in the cover letter > of this series, and they have just been waiting for about a year for > their dependency (QAPI aliases) to be merged. > > If we don't do aliases, I'll have to rework them to do everything in > code instead. It's doable, even if the chardev code wouldn't shrink as > nicely as with the current patches. I just need to know whether it has > to be done or not. > >> >> Now the question that matters for this series: how can QAPI aliases >> >> help us with the things discussed above? >> > >> > The translation needs to be implemented somehow. The obvious way is just >> > writing, reviewing and maintaining code that manually translates. (We >> > don't have this code yet for a fully QAPIfied -chardev. We only have >> > code that bypasses QAPI, i.e. translates to the wrong target format.) >> > >> > Aliases help because they allow reducing the amount of code in favour of >> > data, the alias declarations in the schema. >> >> Understood. >> >> >> If we use the flat variation just internally, say for -chardev's dotted >> >> keys argument, then introspection is not needed. We'd use "with >> >> aliases" just for translating -chardev's dotted keys argument. >> > >> > Note that we're doing more translations that just flattening with >> > aliases, some options have different names in QMP and the CLI. >> > >> > But either way, yes, alias support could be optional so that you need to >> > explicitly enable it when creating a visitor, and then only the CLI code >> > could actually enable it. >> > >> > Limits the possible future uses for the new tool in our toolbox, but is >> > sufficient for -chardev. We can always extend support for it later >> > if/when we actually want to make use of it outside of the CLI. >> >> Yes. >> >> >> Valid argument. CLI with JSON argument should match QMP. Even when >> >> that means both are excessively nested. >> >> >> >> CLI with dotted keys is a different matter, in my opinion. >> > >> > I'm skipping quite a bit of text here, mainly because I think we need an >> > answer first if we really want to switch the keyval options to be >> > HMP-like in more than just the support status (see above). >> > >> > Obviously, the other relevant question would then be if it also ends up >> > like HMP in that most interesting functionality isn't even available and >> > you're forced to use QMP/JSON when you're serious. >> >> I figure this is because we have let "QMP first" degenerate into "QMP >> first, HMP never" sometimes, and we did so mostly because HMP is extra >> work that was hard to justify. >> >> What if you could get the 1:1 HMP command basically for free? It may >> not be the most human-friendly command possible. But it should be a >> start. > > How would you do this? > > The obvious way is mapping QMP 1:1 like we do for some of the command > line options. But you just argued that this is not what you would prefer > for the command line, so it's probably not what you have in mind for HMP > either? I think there are QMP commands where a 1:1 HMP command would be just fine. For others, it would be awkward, but that could still better than nothing. Infrastructure we build to QAPIfy the command line could probably be used to get less awkward HMP commands without writing so much code. Note I'm not talking about HMP commands somebody *wants* to write, say to provide convenience magic that is inappropriate for QMP, or specialized higher level commands where QMP only provides general building blocks. >> > I guess I can go into more details once we have answered these >> > fundamental questions. >> > >> > (We could and should have talked about them and about whether you want >> > to have aliases at all a year ago, before going into detailed code >> > review and making error messages perfect in code that we might throw >> > away anyway...) >> >> I doubt you could possibly be more right than that! > > So what questions need to be answered before we can make a decision? > > You talked a lot about how you like QMP to be different from the command > line. So is restricting aliases to the command line enough to make them > acceptable? Or do we need larger changes? Should I just throw them away > and write translation code for -chardev manually? Fair question, but I'm out of time for right now. Let's continue tomorrow.
Am 06.10.2021 um 15:11 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 05.10.2021 um 15:49 hat Markus Armbruster geschrieben: > >> Kevin Wolf <kwolf@redhat.com> writes: > >> > >> > Am 02.10.2021 um 15:33 hat Markus Armbruster geschrieben: > >> >> I apologize for this wall of text. It's a desparate attempt to cut > >> >> through the complexity and my confusion, and make sense of the actual > >> >> problems we're trying to solve. > >> >> > >> >> So, what problems exactly are we trying to solve? > >> > > >> > I'll start with replying to your final question because I think it's > >> > more helpful to start with the big picture than with details. > >> > > >> > So tools like libvirt want to have a single consistent interface to > >> > configure things on startup and at runtime. We also intend to support > >> > configuration files that should this time support all of the options and > >> > not just a few chosen ones. > >> > >> Yes. > >> > >> > The hypothesis is that QAPIfying the command line is the correct > >> > solution for both of these problems, i.e. all available command line > >> > options must be present in the QAPI schema and will be processed by a > >> > single parser shared with QMP to make sure they are consistent. > >> > >> Yes. > >> > >> This leads us to JSON option arguments and configuration files. > >> Well-suited for management applications that already use QMP. > >> > >> > Adding QAPIfied versions of individual command line options are steps > >> > towards this goal. As soon as they exist for every option, the final > >> > conversion from an open coded getopt() loop (or in fact a hand crafted > >> > parser in the case of vl.c) to something generated from the QAPI schema > >> > should be reasonably easy. > >> > >> Yes. > >> > >> > You're right that adding a second JSON-based command line interface for > >> > every option can already achieve the goal of providing a unified > >> > external interface, at the cost of (interface and code) duplication. Is > >> > this duplication desirable? Certainly no. Is it acceptable? You might > >> > get different opinions on this one. > >> > >> We'd certainly prefer CLI options to match corresponding QMP commands > >> exactly. > >> > >> Unfortunately, existing CLI options deviate from corresponding QMP > >> commands, and existing CLI options without corresponding QMP commands > >> may violate QMP design rules. > >> > >> Note: these issues pertain to human-friendly option syntax. The > >> machine-friendly option syntax is still limited to just a few options, > >> and it does match QMP there. > > > > On the other hand, we only have a handful of existing options that are > > very complex. Most of them aren't even structured and just take a single > > scalar value. So I'm relatively sure that we know the big ones, and > > we're working on them. > > > > Another point here is that I would argue that there is a difference > > between existing options and human-friendly options. Not all existing > > options are human-friendly just because they aren't machine friendly > > either, and there is probably potential for human-friendly syntax that > > doesn't exist yet. > > > > So I would treat support for existing options (i.e. compatibility) and > > support for human-friendly options (i.e. usability) as two separate > > problems. > > That's fair. > > Instead of "human-friendly option syntax", I could've written "our > traditional option syntax (which is better suited to humans than to > machines, although it still fails to be truly friendly to anyone)", but > that's a tad long, isn't it? It is, but I had the impression that you used the same term for both things without making a distinction. > >> > In my opinion, we should try to get rid of hand crafted parsers where > >> > it's reasonably possible, and take advantage of the single unified > >> > option structure that QAPI provides. -chardev specifically has a hand > >> > crafted parser that essentially duplicates the automatically generated > >> > QAPI visitor code, except for the nesting and some differences in option > >> > names. > >> > >> We should definitely parse JSON option arguments with the QAPI > >> machinery, and nothing more. > > > > Yes, no doubt. (And we don't even consistently do that yet, like > > device-add going through QemuOpts after going through QAPI and throwing > > away all type information and silently ignoring anything it doesn't know > > to handle.) > > At least device-add is the last remaining user of 'gen': false. > > The 'any' type also leaves type checking to handwritten code, but at > least it doesn't throw away type information first. Used by qom-get and > qom-set. Even if device-add wasn't 'gen': false, in theory nothing would stop it from going back from the QAPI C object through a output visitor and QemuOpts and throwing the type information away again. Of course, with explicit type checks in QAPI, we're less likely to mess up the other side because otherwise things just wouldn't work. But I actually know a subsystem where such conversions happen... *cough* > >> > HMP in contrast means separate code for every command, or translated to > >> > the CLI for each command line option. HMP is not defined in QAPI, it's > >> > a separate thing that just happens to call into QAPI-based QMP code in > >> > the common case. > >> > > >> > Is this what you have in mind for non-JSON command line options? > >> > >> We should separate the idea "HMP wraps around QMP" from its > >> implementation as handwritten, boilerplate-heavy code. > >> > >> All but the latest, QAPI-based CLI options work pretty much like HMP: a > >> bit of code generation (hxtool), a mix of common and ad hoc parsers, > >> boring handwritten code to translate from parse tree to internal C > >> interfaces (which are often QMP command handlers), and to stitch it all > >> together. > >> > >> Reducing the amount of boring handwritten code is equally desirable for > >> HMP and CLI. > >> > >> So is specifying the interface in QAPI instead of older, less powerful > >> schema languages like hxtool and QemuOpts. > >> > >> There are at least three problems: > >> > >> 1. Specifying CLI in QAPI hasn't progressed beyond the proof-of-concept > >> stage. > >> > >> 2. Backward compatibility issues and doubts have defeated attempts to > >> replace gnarly stuff, in particular QemuOpts. > >> > >> 3. How to best bridge abstract syntax differences has been unclear. > >> Whether the differences are historical accidents or intentional for ease > >> of human use doesn't matter. > >> > >> Aliases feel like a contribution towards solving 3. > >> > >> I don't think 1. is particularly difficult. It's just a big chunk of > >> work that isn't really useful without solutions for 2. and 3. > >> > >> To me, 2. feels intractable head on. Perhaps we better bypass the > >> problem by weakening the compatibility promise like we did for HMP. > > > > Can we define 2 in a bit more specific terms? I feel the biggest part > > of 2 is actually 3. > > > > You mention QemuOpts, but how commonly are the potentially problematic > > features of it even used? Short booleans are deprecated and could be > > dropped in 6.2. merge_lists may or may not have to be replicated. > > I know building lists from repeated keys is said to be a thing, where > > does it happen? I've worked on some of the big ones (blockdev, chardev, > > object, device) and haven't come across it yet. > > Alright, I'll look for you :) Check out commit 2d6dcbf93fb, 396f935a9af, > 54cb65d8588, f1672e6f2b6. These are fairly recent, which is kind of > depressing. There may be more. Should we patch QemuOpts to disallow this by default and require setting QemuOptsList.legacy_enable_repeated_options just to avoid new instances sneaking in? > When I wrote 2., I was specifically thinking of -object, where we > decided not to mess the human-friendly syntax in part because we didn't > fully understand what a conversion from QemuOpts to keyval could break. I seem to remember we were confident that only the various list hacks would be problematic. They only property in -object that makes use of them is memory-backend.host-nodes. We should deprecate the old syntax for it and move on. > > Can we have an intermediate state where CLI parsing involves both QAPI > > generated options and manually written ones? So that we can actually put > > the QAPIfication work already done to use instead of just having a > > promise that it will make things possible eventually, in a few years? > > We kind of sort of have that already: a few option arguments are > QAPI-based. > > Perhaps it's time to crack 1., but in a way that accommodates > handwritten options. Could be like "this option takes a string > argument, and the function to process the option is not to be generated, > only called (just like 'gen': false for commands). We may want to make > introspection describe the argument as "unknown", to distinguish it from > genuine string arguments. Maybe 'data': 'any' rather than 'gen': false if the keyval parser makes sense for the option. Or as a first step, just call into a single C function for any unknown option so that we don't have to split the big switch block immediately and can instead reduce it incrementally. Then 'data': 'any' is an intermediate step we can take when a function is too complicated to be fully QAPIfied in a single commit. But yes, I guess cracking 1. is what I had in mind: Let QAPI control the command line parsing and only call out into handwritten code for not yet converted options. I think this would be a big step forward compared to the current state of only calling into QAPI for selected options because it makes it very obvious that new code should use QAPI, and it would also give us more a visible list of things that still need to be converted. > >> > What I had in mind was using the schema to generate the necessary code, > >> > using the generic keyval parser everywhere, and just providing a hook > >> > where the QDict could be modified after the keyval parser and before the > >> > visitor. Most command line options would not have any explicit code for > >> > parsing input, only the declarative schema and the final option handler > >> > getting a QAPI type (which could actually be the corresponding QMP > >> > command handler in the common case). > >> > > >> > I think these are very different ideas. If we want HMP-like, then all > >> > the keyval based code paths we've built so far don't make much sense. > >> > >> I'm not sure the differences are "very" :) > >> > >> I think you start at "human-friendly and machine-friendly should use the > >> abstract syntax defined in the QAPI schema, except where human-friendly > >> has to differ, say for backward compatibility". > >> > >> This approach considers differences a blemish. The "standard" abstract > >> syntax (the one defined in the QAPI schema) should be accepted in > >> addition to the "alternate" one whenever possible, so "modern" use can > >> avoid the blemishes, and blemishes can be removed once they have fallen > >> out of use. > >> > >> "Alternate" should not be limited to human-friendly. Not limiting keeps > >> things consistent, which is good, because differences are bad. > >> > >> Is that a fair description? > > > > Hm, yes and no. > > > > > > I do think that making the overlap between "standard" and "alternate" > > abstract syntax as big as possible is a good thing because it means less > > translation has to go on between them, and ultimately the interfaces are > > more consistent with each other which actually improves the human > > friendliness for people who get in touch with both syntaxes. > > > > The -chardev conversion mostly deals with differences that I do consider > > a blemish: There is no real reason why options need to have different > > names on both interfaces, and there is also a lot of nesting in QMP for > > which there is no real reason. > > > > The reason why we need to accept both is compatibility, not usability. > > Compatibility requires us to accept both, but each in its place. It > doesn't actually require us to accept both in either place. Yes. > > There are also some differences that are justified by friendliness. > > Having "addr" as a nested struct in QMP just makes sense, and on the > > command line having it flattened makes more sense. > > > > So accepting "path" for "device" in ChardevHostdev is probably a case > > where switching both QMP and CLI to "modern" use and remove the > > "blemish" eventually makes sense to me. The nesting for "addr" isn't. > > > > We may even add more differences to make things human friendly. My > > standard for this is whether the difference serves only for > > compatibility (should go away eventually) or usability (should stay). > > In theory, cleaning up QMP is nice. In practice, we don't have a good > way to rename or move members. When we do, we have to make both old and > new optional, even when you actually have to specify exactly one. This > isn't the end of the world, but it is trading one kind of blemish for > another. > > Improving on that would involve a non-trivial extension to > introspection. Pain right away, gain only when all management > applications that matter grok it. > > I think the realistic assumption is that QMP blemishes stay. > > Of course, we'd prefer not to copy existing QMP blemishes into new > interfaces. That's why we have SocketAddress for new code, and > SocketAddressLegacy for old code. Workable for simple utility types. I > don't have a good solution for "bigger" types. If this is a common problem and we want to solve it in a declarative way, presumably a way to define mappings similar to aliases is not unthinkable. Apart from the effort to implement it, there is no reason why two types on the external interface couldn't share a single representation in C, just with different mappings. So far I think we only did this for SocketAddressLegacy, though, so it might not be a very high priority. > > Now with this, it's still open whether the "standard" syntax should only > > be supported in QMP or also in the CLI, and whether "alternate" syntax > > should only be supported in the CLI or also in QMP. > > > > Is usability actually improved by rejecting the "standard" syntax on the > > command line, or by rejecting "alternate" syntax in QMP? Hardly so. It's > > also not compatibility. So what is the justification for the difference? > > Maintainability? I honestly don't see the improvement there either. > > > > So I don't really see a reason for differences, but at the same time > > it's also not a very important question to me. If you prefer restricting > > things, so be it. > > I dislike adding "alternate" to machine-friendly QMP / CLI poses, > because: > > * Adding "alternate" poses documentation and introspection problems. > The introspection solutions will then necessitate management > application updates. Anything we do becomes ABI immediately, so we > better get it right on the first try. > > * Ignoring the documentation problems is a bad idea, because > undocumented interfaces are. > > * If we ignore the introspection problems, then machines are well > advised to stick to introspectable "standard". Why add "alternate" > then? Complexity for nothing. > > * Adding "alternate" to existing interfaces is pretty much pointless. > To use "alternate", machines first have to check whether this QEMU > provides it, and if not, fall back to "standard". Just use "standard" > and call it a day. > > * For new interfaces, the differences between "standard" and "alternate" > are hopefully smaller than for old interfaces. Machines will likely > not care for improvements "alternate" may provide over "standard". > > I haven't made up my mind on adding "standard" to human-friendly CLI. Removing "alternate" from QMP is easy, at least as long as we don't want to have a different set of "alternate" still enabled in QMP. Removing "standard" from the CLI is much harder. If we can avoid this, let's please avoid it. > Too many ways to do the same thing are bound to confuse. We could end > up with the standard way, the alternate way we provide for backward > compatibility, the alternate way we provide for usability, and any > combination thereof. > > Each of the three ways has its uses, though. Combinations not so much. > > Adding "standard" to human-friendly CLI only poses documentation > problems (there is no introspection). If we limit the backward > compatibility promise to machine-friendly, getting things wrong is no > big deal. That's the plan, but it requires getting machine friendly as a first step so that there is a stable alternative and we can actually lower the backwards compatibility promise for the existing syntax. > >> I start at "human-friendly syntax should be as friendly as we can make > >> it, except where we have to compromise, say for backward compatibility". > >> > >> This approach embraces differences. Mind, not differences just for > >> differences sake, only where they help users. > >> > >> Additionally accepting the "standard" abstract syntax is useful only > >> where it helps users. > >> > >> "Alternate" should be limited to human-friendly. > > > > I think there is a small inconsistency in this reasoning: You say that > > differences must help users, but then this is not the measuring stick > > you use in the next paragraph. If it were, you would be asking whether > > rejecting "standard" abstract syntax helps users, rather than whether > > adding it does. (Even more so because rejecting it is more work! Not > > much more work, but it adds a little bit of complexity.) > > Additionally accepting "standard" complicates the interface and its > documentation. Whether this helps users more than it hurts them is not > obvious. Fair. When it's unclear for users, let's consider developers? :-) I really don't want to invest a lot of time and effort (and code complexity) just to get "standard" rejected. > > So it seems that in practice your approach is more like "different by > > default, making it the same needs justification", whereas I am leaning > > more towards "same by default, making it different needs justification". > > > > Your idea of "human-friendly syntax should be as friendly as we can make > > it" isn't in conflict with either approach. The thing that the idea > > might actually conflict with is our time budget. > > Reasonable people often have quite different ideas on "human-friendly". > > >> Different approaches, without doubt. Yet both have to deal with > >> differences, and both could use similar techniques and machinery for > >> that. You're proposing to do the bulk of the work with aliases, and to > >> have a tree-transforming hook for the remainder. Makes sense to me. > >> However, in sufficiently gnarly cases, we might have to bypass all this > >> and keep using handwritten code until the backward compatibility promise > >> is history: see 2. above. > > > > Possibly. I honestly can't tell how many of these cases we will have. > > In all of -object, we had exactly one problematic option. This could > > easily be handled in a tree-transforming hook. > > > >> In addition each approach has its own, special needs. > >> > >> Yours adds "alternate" syntax to QMP. This poses documentation and > >> introspection problems. The introspection solutions will then > >> necessitate management application updates. > >> > >> Mine trades these problems for others, namely how to generate parsers > >> for two different syntaxes from one QAPI schema. > >> > >> Do I make sense? > > > > In the long run, won't we need documentation and introspection even for > > things that are limited to the command line? Introspection is one of the > > big features enabled by CLI QAPIfication. > > CLI introspection is about the machine-friendly syntax, not the > human-friendly one, just like monitor introspection is about QMP, not > HMP. Good point. > >> >> Valid argument. CLI with JSON argument should match QMP. Even when > >> >> that means both are excessively nested. > >> >> > >> >> CLI with dotted keys is a different matter, in my opinion. > >> > > >> > I'm skipping quite a bit of text here, mainly because I think we need an > >> > answer first if we really want to switch the keyval options to be > >> > HMP-like in more than just the support status (see above). > >> > > >> > Obviously, the other relevant question would then be if it also ends up > >> > like HMP in that most interesting functionality isn't even available and > >> > you're forced to use QMP/JSON when you're serious. > >> > >> I figure this is because we have let "QMP first" degenerate into "QMP > >> first, HMP never" sometimes, and we did so mostly because HMP is extra > >> work that was hard to justify. > >> > >> What if you could get the 1:1 HMP command basically for free? It may > >> not be the most human-friendly command possible. But it should be a > >> start. > > > > How would you do this? > > > > The obvious way is mapping QMP 1:1 like we do for some of the command > > line options. But you just argued that this is not what you would prefer > > for the command line, so it's probably not what you have in mind for HMP > > either? > > I think there are QMP commands where a 1:1 HMP command would be just > fine. > > For others, it would be awkward, but that could still better than > nothing. > > Infrastructure we build to QAPIfy the command line could probably be > used to get less awkward HMP commands without writing so much code. > > Note I'm not talking about HMP commands somebody *wants* to write, say > to provide convenience magic that is inappropriate for QMP, or > specialized higher level commands where QMP only provides general > building blocks. So basically, if a handwritten HMP handler doesn't exist for a given QMP command, just generate one automatically? Sounds reasonable enough to me. > >> > I guess I can go into more details once we have answered these > >> > fundamental questions. > >> > > >> > (We could and should have talked about them and about whether you want > >> > to have aliases at all a year ago, before going into detailed code > >> > review and making error messages perfect in code that we might throw > >> > away anyway...) > >> > >> I doubt you could possibly be more right than that! > > > > So what questions need to be answered before we can make a decision? > > > > You talked a lot about how you like QMP to be different from the command > > line. So is restricting aliases to the command line enough to make them > > acceptable? Or do we need larger changes? Should I just throw them away > > and write translation code for -chardev manually? > > Fair question, but I'm out of time for right now. Let's continue > tomorrow. :-) Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 06.10.2021 um 15:11 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 05.10.2021 um 15:49 hat Markus Armbruster geschrieben: >> >> Kevin Wolf <kwolf@redhat.com> writes: >> >> >> >> > Am 02.10.2021 um 15:33 hat Markus Armbruster geschrieben: >> >> >> I apologize for this wall of text. It's a desparate attempt to cut >> >> >> through the complexity and my confusion, and make sense of the actual >> >> >> problems we're trying to solve. >> >> >> >> >> >> So, what problems exactly are we trying to solve? >> >> > >> >> > I'll start with replying to your final question because I think it's >> >> > more helpful to start with the big picture than with details. >> >> > >> >> > So tools like libvirt want to have a single consistent interface to >> >> > configure things on startup and at runtime. We also intend to support >> >> > configuration files that should this time support all of the options and >> >> > not just a few chosen ones. >> >> >> >> Yes. >> >> >> >> > The hypothesis is that QAPIfying the command line is the correct >> >> > solution for both of these problems, i.e. all available command line >> >> > options must be present in the QAPI schema and will be processed by a >> >> > single parser shared with QMP to make sure they are consistent. >> >> >> >> Yes. >> >> >> >> This leads us to JSON option arguments and configuration files. >> >> Well-suited for management applications that already use QMP. >> >> >> >> > Adding QAPIfied versions of individual command line options are steps >> >> > towards this goal. As soon as they exist for every option, the final >> >> > conversion from an open coded getopt() loop (or in fact a hand crafted >> >> > parser in the case of vl.c) to something generated from the QAPI schema >> >> > should be reasonably easy. >> >> >> >> Yes. >> >> >> >> > You're right that adding a second JSON-based command line interface for >> >> > every option can already achieve the goal of providing a unified >> >> > external interface, at the cost of (interface and code) duplication. Is >> >> > this duplication desirable? Certainly no. Is it acceptable? You might >> >> > get different opinions on this one. >> >> >> >> We'd certainly prefer CLI options to match corresponding QMP commands >> >> exactly. >> >> >> >> Unfortunately, existing CLI options deviate from corresponding QMP >> >> commands, and existing CLI options without corresponding QMP commands >> >> may violate QMP design rules. >> >> >> >> Note: these issues pertain to human-friendly option syntax. The >> >> machine-friendly option syntax is still limited to just a few options, >> >> and it does match QMP there. >> > >> > On the other hand, we only have a handful of existing options that are >> > very complex. Most of them aren't even structured and just take a single >> > scalar value. So I'm relatively sure that we know the big ones, and >> > we're working on them. >> > >> > Another point here is that I would argue that there is a difference >> > between existing options and human-friendly options. Not all existing >> > options are human-friendly just because they aren't machine friendly >> > either, and there is probably potential for human-friendly syntax that >> > doesn't exist yet. >> > >> > So I would treat support for existing options (i.e. compatibility) and >> > support for human-friendly options (i.e. usability) as two separate >> > problems. >> >> That's fair. >> >> Instead of "human-friendly option syntax", I could've written "our >> traditional option syntax (which is better suited to humans than to >> machines, although it still fails to be truly friendly to anyone)", but >> that's a tad long, isn't it? > > It is, but I had the impression that you used the same term for both > things without making a distinction. Unclear writing is usually a symptom of unclear thinking :) >> >> > In my opinion, we should try to get rid of hand crafted parsers where >> >> > it's reasonably possible, and take advantage of the single unified >> >> > option structure that QAPI provides. -chardev specifically has a hand >> >> > crafted parser that essentially duplicates the automatically generated >> >> > QAPI visitor code, except for the nesting and some differences in option >> >> > names. >> >> >> >> We should definitely parse JSON option arguments with the QAPI >> >> machinery, and nothing more. >> > >> > Yes, no doubt. (And we don't even consistently do that yet, like >> > device-add going through QemuOpts after going through QAPI and throwing >> > away all type information and silently ignoring anything it doesn't know >> > to handle.) >> >> At least device-add is the last remaining user of 'gen': false. >> >> The 'any' type also leaves type checking to handwritten code, but at >> least it doesn't throw away type information first. Used by qom-get and >> qom-set. > > Even if device-add wasn't 'gen': false, in theory nothing would stop it > from going back from the QAPI C object through a output visitor and > QemuOpts and throwing the type information away again. Of course, with > explicit type checks in QAPI, we're less likely to mess up the other > side because otherwise things just wouldn't work. > > But I actually know a subsystem where such conversions happen... *cough* Lalalala, nothing to see here, move on... >> >> > HMP in contrast means separate code for every command, or translated to >> >> > the CLI for each command line option. HMP is not defined in QAPI, it's >> >> > a separate thing that just happens to call into QAPI-based QMP code in >> >> > the common case. >> >> > >> >> > Is this what you have in mind for non-JSON command line options? >> >> >> >> We should separate the idea "HMP wraps around QMP" from its >> >> implementation as handwritten, boilerplate-heavy code. >> >> >> >> All but the latest, QAPI-based CLI options work pretty much like HMP: a >> >> bit of code generation (hxtool), a mix of common and ad hoc parsers, >> >> boring handwritten code to translate from parse tree to internal C >> >> interfaces (which are often QMP command handlers), and to stitch it all >> >> together. >> >> >> >> Reducing the amount of boring handwritten code is equally desirable for >> >> HMP and CLI. >> >> >> >> So is specifying the interface in QAPI instead of older, less powerful >> >> schema languages like hxtool and QemuOpts. >> >> >> >> There are at least three problems: >> >> >> >> 1. Specifying CLI in QAPI hasn't progressed beyond the proof-of-concept >> >> stage. >> >> >> >> 2. Backward compatibility issues and doubts have defeated attempts to >> >> replace gnarly stuff, in particular QemuOpts. >> >> >> >> 3. How to best bridge abstract syntax differences has been unclear. >> >> Whether the differences are historical accidents or intentional for ease >> >> of human use doesn't matter. >> >> >> >> Aliases feel like a contribution towards solving 3. >> >> >> >> I don't think 1. is particularly difficult. It's just a big chunk of >> >> work that isn't really useful without solutions for 2. and 3. >> >> >> >> To me, 2. feels intractable head on. Perhaps we better bypass the >> >> problem by weakening the compatibility promise like we did for HMP. >> > >> > Can we define 2 in a bit more specific terms? I feel the biggest part >> > of 2 is actually 3. >> > >> > You mention QemuOpts, but how commonly are the potentially problematic >> > features of it even used? Short booleans are deprecated and could be >> > dropped in 6.2. merge_lists may or may not have to be replicated. >> > I know building lists from repeated keys is said to be a thing, where >> > does it happen? I've worked on some of the big ones (blockdev, chardev, >> > object, device) and haven't come across it yet. >> >> Alright, I'll look for you :) Check out commit 2d6dcbf93fb, 396f935a9af, >> 54cb65d8588, f1672e6f2b6. These are fairly recent, which is kind of >> depressing. There may be more. > > Should we patch QemuOpts to disallow this by default and require setting > QemuOptsList.legacy_enable_repeated_options just to avoid new instances > sneaking in? I honestly don't know. On the one hand, the use of repeated keys for lists wasn't designed into QemuOpts, it was a "clever" (ab)use of a QemuOpts implementation detail. On the other hand, -vnc vnc=localhost:1,vnc=unix:/tmp/vnc feels friendlier to human me than -vnc vnc.0=localhost:1,vnc.1=unix:/tmp/vnc. Repeated keys just don't fit into the QAPI parsing pipeline. "Object member names are unique" is baked into data structures and code. Quite reasonable when you start with JSON. But this is a digression. >> When I wrote 2., I was specifically thinking of -object, where we >> decided not to mess the human-friendly syntax in part because we didn't >> fully understand what a conversion from QemuOpts to keyval could break. > > I seem to remember we were confident that only the various list hacks > would be problematic. They only property in -object that makes use of > them is memory-backend.host-nodes. We should deprecate the old syntax > for it and move on. Yes, we should work on an orderly transition away from QemuOpts. >> > Can we have an intermediate state where CLI parsing involves both QAPI >> > generated options and manually written ones? So that we can actually put >> > the QAPIfication work already done to use instead of just having a >> > promise that it will make things possible eventually, in a few years? >> >> We kind of sort of have that already: a few option arguments are >> QAPI-based. >> >> Perhaps it's time to crack 1., but in a way that accommodates >> handwritten options. Could be like "this option takes a string >> argument, and the function to process the option is not to be generated, >> only called (just like 'gen': false for commands). We may want to make >> introspection describe the argument as "unknown", to distinguish it from >> genuine string arguments. > > Maybe 'data': 'any' rather than 'gen': false if the keyval parser makes > sense for the option. Yes, providing for "keyval without visitor" may be useful. > Or as a first step, just call into a single C > function for any unknown option so that we don't have to split the big > switch block immediately and can instead reduce it incrementally. Then > 'data': 'any' is an intermediate step we can take when a function is too > complicated to be fully QAPIfied in a single commit. Yes, we could have some options defined in the QAPI schema, the remainder in .hx, then stitch the generated code together. Enables moving commands from .hx to the QAPI schema one by one. I'm not sure that's simpler than replacing .hx wholesale. > But yes, I guess cracking 1. is what I had in mind: Let QAPI control the > command line parsing and only call out into handwritten code for not yet > converted options. I think this would be a big step forward compared to > the current state of only calling into QAPI for selected options because > it makes it very obvious that new code should use QAPI, and it would > also give us more a visible list of things that still need to be > converted. No argument. I have (dusty) RFC patches. >> >> > What I had in mind was using the schema to generate the necessary code, >> >> > using the generic keyval parser everywhere, and just providing a hook >> >> > where the QDict could be modified after the keyval parser and before the >> >> > visitor. Most command line options would not have any explicit code for >> >> > parsing input, only the declarative schema and the final option handler >> >> > getting a QAPI type (which could actually be the corresponding QMP >> >> > command handler in the common case). >> >> > >> >> > I think these are very different ideas. If we want HMP-like, then all >> >> > the keyval based code paths we've built so far don't make much sense. >> >> >> >> I'm not sure the differences are "very" :) >> >> >> >> I think you start at "human-friendly and machine-friendly should use the >> >> abstract syntax defined in the QAPI schema, except where human-friendly >> >> has to differ, say for backward compatibility". >> >> >> >> This approach considers differences a blemish. The "standard" abstract >> >> syntax (the one defined in the QAPI schema) should be accepted in >> >> addition to the "alternate" one whenever possible, so "modern" use can >> >> avoid the blemishes, and blemishes can be removed once they have fallen >> >> out of use. >> >> >> >> "Alternate" should not be limited to human-friendly. Not limiting keeps >> >> things consistent, which is good, because differences are bad. >> >> >> >> Is that a fair description? >> > >> > Hm, yes and no. >> > >> > >> > I do think that making the overlap between "standard" and "alternate" >> > abstract syntax as big as possible is a good thing because it means less >> > translation has to go on between them, and ultimately the interfaces are >> > more consistent with each other which actually improves the human >> > friendliness for people who get in touch with both syntaxes. >> > >> > The -chardev conversion mostly deals with differences that I do consider >> > a blemish: There is no real reason why options need to have different >> > names on both interfaces, and there is also a lot of nesting in QMP for >> > which there is no real reason. >> > >> > The reason why we need to accept both is compatibility, not usability. >> >> Compatibility requires us to accept both, but each in its place. It >> doesn't actually require us to accept both in either place. > > Yes. > >> > There are also some differences that are justified by friendliness. >> > Having "addr" as a nested struct in QMP just makes sense, and on the >> > command line having it flattened makes more sense. >> > >> > So accepting "path" for "device" in ChardevHostdev is probably a case >> > where switching both QMP and CLI to "modern" use and remove the >> > "blemish" eventually makes sense to me. The nesting for "addr" isn't. >> > >> > We may even add more differences to make things human friendly. My >> > standard for this is whether the difference serves only for >> > compatibility (should go away eventually) or usability (should stay). >> >> In theory, cleaning up QMP is nice. In practice, we don't have a good >> way to rename or move members. When we do, we have to make both old and >> new optional, even when you actually have to specify exactly one. This >> isn't the end of the world, but it is trading one kind of blemish for >> another. >> >> Improving on that would involve a non-trivial extension to >> introspection. Pain right away, gain only when all management >> applications that matter grok it. >> >> I think the realistic assumption is that QMP blemishes stay. >> >> Of course, we'd prefer not to copy existing QMP blemishes into new >> interfaces. That's why we have SocketAddress for new code, and >> SocketAddressLegacy for old code. Workable for simple utility types. I >> don't have a good solution for "bigger" types. > > If this is a common problem and we want to solve it in a declarative > way, presumably a way to define mappings similar to aliases is not > unthinkable. Apart from the effort to implement it, there is no reason > why two types on the external interface couldn't share a single > representation in C, just with different mappings. True. If we use aliases to provide multiple wire formats for a common C type, the common C type can only be the most nested one. I'd prefer the least nested one. We'll live. > So far I think we only did this for SocketAddressLegacy, though, so it > might not be a very high priority. Yes. >> > Now with this, it's still open whether the "standard" syntax should only >> > be supported in QMP or also in the CLI, and whether "alternate" syntax >> > should only be supported in the CLI or also in QMP. >> > >> > Is usability actually improved by rejecting the "standard" syntax on the >> > command line, or by rejecting "alternate" syntax in QMP? Hardly so. It's >> > also not compatibility. So what is the justification for the difference? >> > Maintainability? I honestly don't see the improvement there either. >> > >> > So I don't really see a reason for differences, but at the same time >> > it's also not a very important question to me. If you prefer restricting >> > things, so be it. >> >> I dislike adding "alternate" to machine-friendly QMP / CLI poses, >> because: >> >> * Adding "alternate" poses documentation and introspection problems. >> The introspection solutions will then necessitate management >> application updates. Anything we do becomes ABI immediately, so we >> better get it right on the first try. >> >> * Ignoring the documentation problems is a bad idea, because >> undocumented interfaces are. >> >> * If we ignore the introspection problems, then machines are well >> advised to stick to introspectable "standard". Why add "alternate" >> then? Complexity for nothing. >> >> * Adding "alternate" to existing interfaces is pretty much pointless. >> To use "alternate", machines first have to check whether this QEMU >> provides it, and if not, fall back to "standard". Just use "standard" >> and call it a day. >> >> * For new interfaces, the differences between "standard" and "alternate" >> are hopefully smaller than for old interfaces. Machines will likely >> not care for improvements "alternate" may provide over "standard". >> >> I haven't made up my mind on adding "standard" to human-friendly CLI. > > Removing "alternate" from QMP is easy, at least as long as we don't want > to have a different set of "alternate" still enabled in QMP. > > Removing "standard" from the CLI is much harder. If we can avoid this, > let's please avoid it. Noted. >> Too many ways to do the same thing are bound to confuse. We could end >> up with the standard way, the alternate way we provide for backward >> compatibility, the alternate way we provide for usability, and any >> combination thereof. >> >> Each of the three ways has its uses, though. Combinations not so much. >> >> Adding "standard" to human-friendly CLI only poses documentation >> problems (there is no introspection). If we limit the backward >> compatibility promise to machine-friendly, getting things wrong is no >> big deal. > > That's the plan, but it requires getting machine friendly as a first > step so that there is a stable alternative and we can actually lower the > backwards compatibility promise for the existing syntax. True. >> >> I start at "human-friendly syntax should be as friendly as we can make >> >> it, except where we have to compromise, say for backward compatibility". >> >> >> >> This approach embraces differences. Mind, not differences just for >> >> differences sake, only where they help users. >> >> >> >> Additionally accepting the "standard" abstract syntax is useful only >> >> where it helps users. >> >> >> >> "Alternate" should be limited to human-friendly. >> > >> > I think there is a small inconsistency in this reasoning: You say that >> > differences must help users, but then this is not the measuring stick >> > you use in the next paragraph. If it were, you would be asking whether >> > rejecting "standard" abstract syntax helps users, rather than whether >> > adding it does. (Even more so because rejecting it is more work! Not >> > much more work, but it adds a little bit of complexity.) >> >> Additionally accepting "standard" complicates the interface and its >> documentation. Whether this helps users more than it hurts them is not >> obvious. > > Fair. When it's unclear for users, let's consider developers? :-) > > I really don't want to invest a lot of time and effort (and code > complexity) just to get "standard" rejected. I acknowledge the existence of working code :) If we had understood the problem back then as well as we do now, you might have written different code. Aliases are for *adding* "alternate" to "standard" by nature. To get "alternate" *instead* of "standard", something else might be a better fit. "T2 inherits from T1 with the following renames" comes to mind. >> > So it seems that in practice your approach is more like "different by >> > default, making it the same needs justification", whereas I am leaning >> > more towards "same by default, making it different needs justification". >> > >> > Your idea of "human-friendly syntax should be as friendly as we can make >> > it" isn't in conflict with either approach. The thing that the idea >> > might actually conflict with is our time budget. >> >> Reasonable people often have quite different ideas on "human-friendly". >> >> >> Different approaches, without doubt. Yet both have to deal with >> >> differences, and both could use similar techniques and machinery for >> >> that. You're proposing to do the bulk of the work with aliases, and to >> >> have a tree-transforming hook for the remainder. Makes sense to me. >> >> However, in sufficiently gnarly cases, we might have to bypass all this >> >> and keep using handwritten code until the backward compatibility promise >> >> is history: see 2. above. >> > >> > Possibly. I honestly can't tell how many of these cases we will have. >> > In all of -object, we had exactly one problematic option. This could >> > easily be handled in a tree-transforming hook. >> > >> >> In addition each approach has its own, special needs. >> >> >> >> Yours adds "alternate" syntax to QMP. This poses documentation and >> >> introspection problems. The introspection solutions will then >> >> necessitate management application updates. >> >> >> >> Mine trades these problems for others, namely how to generate parsers >> >> for two different syntaxes from one QAPI schema. >> >> >> >> Do I make sense? >> > >> > In the long run, won't we need documentation and introspection even for >> > things that are limited to the command line? Introspection is one of the >> > big features enabled by CLI QAPIfication. >> >> CLI introspection is about the machine-friendly syntax, not the >> human-friendly one, just like monitor introspection is about QMP, not >> HMP. > > Good point. > >> >> >> Valid argument. CLI with JSON argument should match QMP. Even when >> >> >> that means both are excessively nested. >> >> >> >> >> >> CLI with dotted keys is a different matter, in my opinion. >> >> > >> >> > I'm skipping quite a bit of text here, mainly because I think we need an >> >> > answer first if we really want to switch the keyval options to be >> >> > HMP-like in more than just the support status (see above). >> >> > >> >> > Obviously, the other relevant question would then be if it also ends up >> >> > like HMP in that most interesting functionality isn't even available and >> >> > you're forced to use QMP/JSON when you're serious. >> >> >> >> I figure this is because we have let "QMP first" degenerate into "QMP >> >> first, HMP never" sometimes, and we did so mostly because HMP is extra >> >> work that was hard to justify. >> >> >> >> What if you could get the 1:1 HMP command basically for free? It may >> >> not be the most human-friendly command possible. But it should be a >> >> start. >> > >> > How would you do this? >> > >> > The obvious way is mapping QMP 1:1 like we do for some of the command >> > line options. But you just argued that this is not what you would prefer >> > for the command line, so it's probably not what you have in mind for HMP >> > either? >> >> I think there are QMP commands where a 1:1 HMP command would be just >> fine. >> >> For others, it would be awkward, but that could still better than >> nothing. >> >> Infrastructure we build to QAPIfy the command line could probably be >> used to get less awkward HMP commands without writing so much code. >> >> Note I'm not talking about HMP commands somebody *wants* to write, say >> to provide convenience magic that is inappropriate for QMP, or >> specialized higher level commands where QMP only provides general >> building blocks. > > So basically, if a handwritten HMP handler doesn't exist for a given QMP > command, just generate one automatically? Sounds reasonable enough to > me. Fun project, but I can't make it mine. >> >> > I guess I can go into more details once we have answered these >> >> > fundamental questions. >> >> > >> >> > (We could and should have talked about them and about whether you want >> >> > to have aliases at all a year ago, before going into detailed code >> >> > review and making error messages perfect in code that we might throw >> >> > away anyway...) >> >> >> >> I doubt you could possibly be more right than that! >> > >> > So what questions need to be answered before we can make a decision? >> > >> > You talked a lot about how you like QMP to be different from the command >> > line. So is restricting aliases to the command line enough to make them >> > acceptable? Or do we need larger changes? Should I just throw them away >> > and write translation code for -chardev manually? >> >> Fair question, but I'm out of time for right now. Let's continue >> tomorrow. > > :-) What's the smallest set of aliases sufficient to make your -chardev QAPIfication work?
© 2016 - 2026 Red Hat, Inc.