* [PATCH 0/6] qapi: Add support for aliases
@ 2020-11-12 17:28 Kevin Wolf
2020-11-12 17:28 ` [PATCH 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
` (7 more replies)
0 siblings, 8 replies; 32+ messages in thread
From: Kevin Wolf @ 2020-11-12 17:28 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow, armbru
This series introduces alias definitions for QAPI object types (structs
and unions).
This allows using the same QAPI type and visitor even when the syntax
has some variations between different external interfaces such as QMP
and the command line.
It also provides a new tool for evolving the schema while maintaining
backwards compatibility (possibly during a deprecation period).
The first user is intended to be a QAPIfied -chardev command line
option, for which I'll send a separate series. A git tag is available
that contains both this series and the chardev changes that make use of
it:
https://repo.or.cz/qemu/kevin.git qapi-alias-chardev-v1
Kevin Wolf (6):
qapi: Add interfaces for alias support to Visitor
qapi: Remember alias definitions in qobject-input-visitor
qapi: Simplify full_name_nth() in qobject-input-visitor
qapi: Apply aliases in qobject-input-visitor
qapi: Add support for aliases
tests/qapi-schema: Test cases for aliases
docs/devel/qapi-code-gen.txt | 37 +-
docs/sphinx/qapidoc.py | 2 +-
include/qapi/visitor-impl.h | 12 +
include/qapi/visitor.h | 37 ++
qapi/qapi-visit-core.c | 21 ++
qapi/qobject-input-visitor.c | 322 ++++++++++++++++--
scripts/qapi/expr.py | 34 +-
scripts/qapi/schema.py | 27 +-
scripts/qapi/types.py | 4 +-
scripts/qapi/visit.py | 33 +-
tests/qapi-schema/test-qapi.py | 7 +-
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-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
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/double-type.err | 2 +-
tests/qapi-schema/meson.build | 8 +
tests/qapi-schema/qapi-schema-test.json | 24 ++
tests/qapi-schema/qapi-schema-test.out | 29 ++
tests/qapi-schema/unknown-expr-key.err | 2 +-
40 files changed, 596 insertions(+), 46 deletions(-)
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-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-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
--
2.28.0
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/6] qapi: Add interfaces for alias support to Visitor
2020-11-12 17:28 [PATCH 0/6] qapi: Add support for aliases Kevin Wolf
@ 2020-11-12 17:28 ` Kevin Wolf
2021-01-26 15:59 ` Markus Armbruster
2021-01-27 12:51 ` Markus Armbruster
2020-11-12 17:28 ` [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor Kevin Wolf
` (6 subsequent siblings)
7 siblings, 2 replies; 32+ messages in thread
From: Kevin Wolf @ 2020-11-12 17:28 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow, armbru
This adds functions to the Visitor interface that can be used to define
aliases and alias scopes.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/qapi/visitor-impl.h | 12 ++++++++++++
include/qapi/visitor.h | 37 +++++++++++++++++++++++++++++++++++++
qapi/qapi-visit-core.c | 21 +++++++++++++++++++++
3 files changed, 70 insertions(+)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7362c043be..e30da2599c 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -113,6 +113,18 @@ struct Visitor
The core takes care of the return type in the public interface. */
void (*optional)(Visitor *v, const char *name, bool *present);
+ /*
+ * Optional; intended for input visitors. If not given, aliases are
+ * ignored.
+ */
+ void (*define_alias)(Visitor *v, const char *alias, const char **source);
+
+ /* Must be set if define_alias is set */
+ void (*start_alias_scope)(Visitor *v);
+
+ /* Must be set if define_alias is set */
+ void (*end_alias_scope)(Visitor *v);
+
/* Must be set */
VisitorType type;
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index ebc19ede7f..9bdc0ee03d 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -459,6 +459,43 @@ void visit_end_alternate(Visitor *v, void **obj);
*/
bool visit_optional(Visitor *v, const char *name, bool *present);
+/*
+ * Defines a new alias rule.
+ *
+ * If @alias is non-NULL, the member named @alias in the external
+ * representation of the current struct is defined as an alias for the
+ * member described by @source.
+ *
+ * If @alias is NULL, all members of the struct described by @source are
+ * considered to have alias members with the same key in the current
+ * struct.
+ *
+ * @source is a NULL-terminated array of names that describe the path to
+ * a member, starting from the currently visited struct.
+ *
+ * The alias stays valid until the current alias scope ends.
+ * visit_start/end_struct() implicitly start/end an alias scope.
+ * Additionally, visit_start/end_alias_scope() can be used to explicitly
+ * create a nested alias scope.
+ */
+void visit_define_alias(Visitor *v, const char *alias, const char **source);
+
+/*
+ * Begins an explicit alias scope.
+ *
+ * Alias definitions after here will only stay valid until the
+ * corresponding visit_end_alias_scope() is called.
+ */
+void visit_start_alias_scope(Visitor *v);
+
+/*
+ * Ends an explicit alias scope.
+ *
+ * Alias definitions between the correspoding visit_start_alias_scope()
+ * call and here go out of scope and won't apply in later code any more.
+ */
+void visit_end_alias_scope(Visitor *v);
+
/*
* Visit an enum value.
*
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 7e5f40e7f0..719a9f5da2 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -135,6 +135,27 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
return *present;
}
+void visit_define_alias(Visitor *v, const char *alias, const char **source)
+{
+ if (v->define_alias) {
+ v->define_alias(v, alias, source);
+ }
+}
+
+void visit_start_alias_scope(Visitor *v)
+{
+ if (v->start_alias_scope) {
+ v->start_alias_scope(v);
+ }
+}
+
+void visit_end_alias_scope(Visitor *v)
+{
+ if (v->end_alias_scope) {
+ v->end_alias_scope(v);
+ }
+}
+
bool visit_is_input(Visitor *v)
{
return v->type == VISITOR_INPUT;
--
2.28.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor
2020-11-12 17:28 [PATCH 0/6] qapi: Add support for aliases Kevin Wolf
2020-11-12 17:28 ` [PATCH 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
@ 2020-11-12 17:28 ` Kevin Wolf
2021-01-27 13:06 ` Markus Armbruster
2021-02-09 12:57 ` Markus Armbruster
2020-11-12 17:28 ` [PATCH 3/6] qapi: Simplify full_name_nth() " Kevin Wolf
` (5 subsequent siblings)
7 siblings, 2 replies; 32+ messages in thread
From: Kevin Wolf @ 2020-11-12 17:28 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow, armbru
This makes qobject-input-visitor remember the currently valid aliases in
each StackObject. It doesn't actually allow using the aliases yet.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++++++++++++
1 file changed, 115 insertions(+)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 23843b242e..a00ac32682 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -29,6 +29,29 @@
#include "qemu/cutils.h"
#include "qemu/option.h"
+typedef struct InputVisitorAlias {
+ /* Alias name. NULL for any (wildcard alias). */
+ const char *alias;
+
+ /*
+ * NULL terminated array representing a path.
+ * Must contain at least one non-NULL element if alias is not NULL.
+ */
+ const char **src;
+
+ /* StackObject in which the alias should be looked for */
+ struct StackObject *alias_so;
+
+ /*
+ * The alias remains valid as long as the containing StackObject has
+ * StackObject.alias_scope_nesting >= InputVisitorAlias.scope_nesting
+ * or until the whole StackObject is removed.
+ */
+ int scope_nesting;
+
+ QSLIST_ENTRY(InputVisitorAlias) next;
+} InputVisitorAlias;
+
typedef struct StackObject {
const char *name; /* Name of @obj in its parent, if any */
QObject *obj; /* QDict or QList being visited */
@@ -38,6 +61,9 @@ typedef struct StackObject {
const QListEntry *entry; /* If @obj is QList: unvisited tail */
unsigned index; /* If @obj is QList: list index of @entry */
+ QSLIST_HEAD(, InputVisitorAlias) aliases;
+ int alias_scope_nesting; /* Increase on scope start, decrease on end */
+
QSLIST_ENTRY(StackObject) node; /* parent */
} StackObject;
@@ -203,6 +229,38 @@ static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv,
return qstring_get_str(qstr);
}
+/*
+ * Propagates aliases from the parent StackObject @src to its direct
+ * child StackObject @dst, which is representing the child struct @name.
+ *
+ * Every alias whose source path begins with @name and which still
+ * applies in @dst (i.e. it is either a wildcard alias or has at least
+ * one more source path element) is propagated to @dst with the first
+ * element (i.e. @name) removed from the source path.
+ */
+static void propagate_aliases(StackObject *dst, StackObject *src,
+ const char *name)
+{
+ InputVisitorAlias *a;
+
+ QSLIST_FOREACH(a, &src->aliases, next) {
+ if (!a->src[0] || strcmp(a->src[0], name)) {
+ continue;
+ }
+ if (a->src[1] || !a->alias) {
+ InputVisitorAlias *alias = g_new(InputVisitorAlias, 1);
+
+ *alias = (InputVisitorAlias) {
+ .alias = a->alias,
+ .alias_so = a->alias_so,
+ .src = &a->src[1],
+ };
+
+ QSLIST_INSERT_HEAD(&dst->aliases, alias, next);
+ }
+ }
+}
+
static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
const char *name,
QObject *obj, void *qapi)
@@ -226,6 +284,9 @@ static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
g_hash_table_insert(h, (void *)qdict_entry_key(entry), NULL);
}
tos->h = h;
+ if (!QSLIST_EMPTY(&qiv->stack)) {
+ propagate_aliases(tos, QSLIST_FIRST(&qiv->stack), name);
+ }
} else {
assert(qlist);
tos->entry = qlist_first(qlist);
@@ -257,10 +318,17 @@ static bool qobject_input_check_struct(Visitor *v, Error **errp)
static void qobject_input_stack_object_free(StackObject *tos)
{
+ InputVisitorAlias *a;
+
if (tos->h) {
g_hash_table_unref(tos->h);
}
+ while ((a = QSLIST_FIRST(&tos->aliases))) {
+ QSLIST_REMOVE_HEAD(&tos->aliases, next);
+ g_free(a);
+ }
+
g_free(tos);
}
@@ -274,6 +342,50 @@ static void qobject_input_pop(Visitor *v, void **obj)
qobject_input_stack_object_free(tos);
}
+static void qobject_input_start_alias_scope(Visitor *v)
+{
+ QObjectInputVisitor *qiv = to_qiv(v);
+ StackObject *tos = QSLIST_FIRST(&qiv->stack);
+
+ tos->alias_scope_nesting++;
+}
+
+static void qobject_input_end_alias_scope(Visitor *v)
+{
+ QObjectInputVisitor *qiv = to_qiv(v);
+ StackObject *tos = QSLIST_FIRST(&qiv->stack);
+ InputVisitorAlias *a, *next;
+
+ assert(tos->alias_scope_nesting > 0);
+ tos->alias_scope_nesting--;
+
+ QSLIST_FOREACH_SAFE(a, &tos->aliases, next, next) {
+ if (a->scope_nesting > tos->alias_scope_nesting) {
+ QSLIST_REMOVE(&tos->aliases, a, InputVisitorAlias, next);
+ g_free(a);
+ }
+ }
+}
+
+static void qobject_input_define_alias(Visitor *v, const char *alias_name,
+ const char **source)
+{
+ QObjectInputVisitor *qiv = to_qiv(v);
+ StackObject *tos = QSLIST_FIRST(&qiv->stack);
+ InputVisitorAlias *alias = g_new(InputVisitorAlias, 1);
+
+ /* The source path can only be empty for wildcard aliases */
+ assert(source[0] || !alias_name);
+
+ *alias = (InputVisitorAlias) {
+ .alias = alias_name,
+ .alias_so = tos,
+ .src = source,
+ };
+
+ QSLIST_INSERT_HEAD(&tos->aliases, alias, next);
+}
+
static bool qobject_input_start_struct(Visitor *v, const char *name, void **obj,
size_t size, Error **errp)
{
@@ -696,6 +808,9 @@ static QObjectInputVisitor *qobject_input_visitor_base_new(QObject *obj)
v->visitor.end_list = qobject_input_end_list;
v->visitor.start_alternate = qobject_input_start_alternate;
v->visitor.optional = qobject_input_optional;
+ v->visitor.define_alias = qobject_input_define_alias;
+ v->visitor.start_alias_scope = qobject_input_start_alias_scope;
+ v->visitor.end_alias_scope = qobject_input_end_alias_scope;
v->visitor.free = qobject_input_free;
v->root = qobject_ref(obj);
--
2.28.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor
2020-11-12 17:28 [PATCH 0/6] qapi: Add support for aliases Kevin Wolf
2020-11-12 17:28 ` [PATCH 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
2020-11-12 17:28 ` [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor Kevin Wolf
@ 2020-11-12 17:28 ` Kevin Wolf
2021-01-27 13:56 ` Markus Armbruster
2020-11-12 17:28 ` [PATCH 4/6] qapi: Apply aliases " Kevin Wolf
` (4 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2020-11-12 17:28 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow, armbru
Instead of counting how many elements from the top of the stack we need
to ignore until we find the thing we're interested in, we can just
directly pass the StackObject pointer because all callers already know
it.
We only need a different way now to tell if we want to know the name of
something contained in the given StackObject or of the StackObject
itself. Passing name = NULL is the obvious way to request the latter.
This simplifies the interface and makes it easier to use in cases where
we have the StackObject, but don't know how many steps down the stack it
is.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/qobject-input-visitor.c | 38 ++++++++++++++++++------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index a00ac32682..1415561828 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -87,20 +87,16 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
}
/*
- * Find the full name of something @qiv is currently visiting.
- * @qiv is visiting something named @name in the stack of containers
- * @qiv->stack.
- * If @n is zero, return its full name.
- * If @n is positive, return the full name of the @n-th container
- * counting from the top. The stack of containers must have at least
- * @n elements.
- * The returned string is valid until the next full_name_nth(@v) or
- * destruction of @v.
+ * Find the full name of something named @name in @so which @qiv is
+ * currently visiting. If @name is NULL, find the full name of @so
+ * itself.
+ *
+ * The returned string is valid until the next full_name_so(@qiv) or
+ * destruction of @qiv.
*/
-static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
- int n)
+static const char *full_name_so(QObjectInputVisitor *qiv, const char *name,
+ StackObject *so)
{
- StackObject *so;
char buf[32];
if (qiv->errname) {
@@ -109,10 +105,13 @@ static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
qiv->errname = g_string_new("");
}
- QSLIST_FOREACH(so , &qiv->stack, node) {
- if (n) {
- n--;
- } else if (qobject_type(so->obj) == QTYPE_QDICT) {
+ if (!name && so) {
+ name = so->name;
+ so = QSLIST_NEXT(so, node);
+ }
+
+ for (; so; so = QSLIST_NEXT(so, node)) {
+ if (qobject_type(so->obj) == QTYPE_QDICT) {
g_string_prepend(qiv->errname, name ?: "<anonymous>");
g_string_prepend_c(qiv->errname, '.');
} else {
@@ -123,7 +122,6 @@ static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
}
name = so->name;
}
- assert(!n);
if (name) {
g_string_prepend(qiv->errname, name);
@@ -138,7 +136,9 @@ static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
static const char *full_name(QObjectInputVisitor *qiv, const char *name)
{
- return full_name_nth(qiv, name, 0);
+ StackObject *tos = QSLIST_FIRST(&qiv->stack);
+
+ return full_name_so(qiv, name, tos);
}
static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
@@ -473,7 +473,7 @@ static bool qobject_input_check_list(Visitor *v, Error **errp)
if (tos->entry) {
error_setg(errp, "Only %u list elements expected in %s",
- tos->index + 1, full_name_nth(qiv, NULL, 1));
+ tos->index + 1, full_name_so(qiv, NULL, tos));
return false;
}
return true;
--
2.28.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 4/6] qapi: Apply aliases in qobject-input-visitor
2020-11-12 17:28 [PATCH 0/6] qapi: Add support for aliases Kevin Wolf
` (2 preceding siblings ...)
2020-11-12 17:28 ` [PATCH 3/6] qapi: Simplify full_name_nth() " Kevin Wolf
@ 2020-11-12 17:28 ` Kevin Wolf
2021-02-09 16:02 ` Markus Armbruster
2020-11-12 17:28 ` [PATCH 5/6] qapi: Add support for aliases Kevin Wolf
` (3 subsequent siblings)
7 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2020-11-12 17:28 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow, armbru
When looking for an object in a struct in the external representation,
check not only the currently visited struct, but also whether an alias
in the current StackObject matches and try to fetch the value from the
alias then. Providing two values for the same object through different
aliases is an error.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/qobject-input-visitor.c | 169 +++++++++++++++++++++++++++++++++--
1 file changed, 160 insertions(+), 9 deletions(-)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 1415561828..faca5b6b55 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -74,6 +74,8 @@ struct QObjectInputVisitor {
QObject *root;
bool keyval; /* Assume @root made with keyval_parse() */
+ QDict *empty_qdict; /* Used for implicit objects */
+
/* Stack of objects being visited (all entries will be either
* QDict or QList). */
QSLIST_HEAD(, StackObject) stack;
@@ -141,9 +143,139 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
return full_name_so(qiv, name, tos);
}
+static bool find_object_member(QObjectInputVisitor *qiv,
+ StackObject **so, const char **name,
+ bool *implicit_object, Error **errp);
+
+static bool alias_present(QObjectInputVisitor *qiv,
+ InputVisitorAlias *a, const char *name)
+{
+ StackObject *so = a->alias_so;
+
+ /*
+ * The passed source @name is only relevant for wildcard aliases which
+ * don't have a separate name, otherwise we use the alias name.
+ */
+ if (a->alias) {
+ name = a->alias;
+ }
+
+ if (!find_object_member(qiv, &so, &name, NULL, NULL)) {
+ return false;
+ }
+
+ /*
+ * Every source can be used only once. If a value in the input would end up
+ * being used twice through aliases, we'll fail the second access.
+ */
+ if (!g_hash_table_contains(so->h, name)) {
+ return false;
+ }
+
+ return true;
+}
+
+static bool alias_source_matches(QObjectInputVisitor *qiv,
+ StackObject *so, InputVisitorAlias *a,
+ const char *name, bool *implicit_object)
+{
+ if (a->src[0] == NULL) {
+ assert(a->alias == NULL);
+ return true;
+ }
+
+ if (!strcmp(a->src[0], name)) {
+ if (a->alias && a->src[1] == NULL) {
+ /*
+ * We're matching an exact member, the source for this alias is
+ * immediately in @so.
+ */
+ return true;
+ } else if (implicit_object) {
+ /*
+ * We're only looking at a prefix of the source path for the alias.
+ * If the input contains no object of the requested name, we will
+ * implicitly create an empty one so that the alias can still be
+ * used.
+ *
+ * We want to create the implicit object only if the alias is
+ * actually used, but we can't tell here for wildcard aliases (only
+ * a later visitor call will determine this). This means that
+ * wildcard aliases must never have optional keys in their source
+ * path.
+ */
+ if (!a->alias || alias_present(qiv, a, a->alias)) {
+ *implicit_object = true;
+ }
+ }
+ }
+
+ return false;
+}
+
+static bool find_object_member(QObjectInputVisitor *qiv,
+ StackObject **so, const char **name,
+ bool *implicit_object, Error **errp)
+{
+ StackObject *cur_so = *so;
+ QDict *qdict = qobject_to(QDict, cur_so->obj);
+ const char *found = NULL;
+ bool found_is_wildcard = false;
+ InputVisitorAlias *a;
+
+ if (implicit_object) {
+ *implicit_object = false;
+ }
+
+ /* Directly present in the container */
+ if (qdict_haskey(qdict, *name)) {
+ found = *name;
+ }
+
+ /*
+ * Find aliases whose source path matches @name in this StackObject. We can
+ * then get the value with the key a->alias from a->alias_so.
+ */
+ QSLIST_FOREACH(a, &cur_so->aliases, next) {
+ if (a->alias == NULL && found) {
+ /*
+ * Skip wildcard aliases if we already have a match. This is
+ * not a conflict that should result in an error.
+ */
+ continue;
+ }
+
+ if (!alias_source_matches(qiv, cur_so, a, *name, implicit_object)) {
+ continue;
+ }
+
+ if (!alias_present(qiv, a, *name)) {
+ continue;
+ }
+
+ if (found && !found_is_wildcard) {
+ error_setg(errp, "Value for parameter %s was already given "
+ "through an alias", full_name_so(qiv, *name, *so));
+ return false;
+ } else {
+ found = a->alias ?: *name;
+ *so = a->alias_so;
+ found_is_wildcard = !a->alias;
+ }
+ }
+
+ /* Chained aliases: *so/found might be the source of another alias */
+ if (found && (*so != cur_so || found != *name)) {
+ find_object_member(qiv, so, &found, NULL, errp);
+ }
+
+ *name = found;
+ return found;
+}
+
static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
const char *name,
- bool consume)
+ bool consume, Error **errp)
{
StackObject *tos;
QObject *qobj;
@@ -161,10 +293,24 @@ static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
assert(qobj);
if (qobject_type(qobj) == QTYPE_QDICT) {
- assert(name);
- ret = qdict_get(qobject_to(QDict, qobj), name);
- if (tos->h && consume && ret) {
- bool removed = g_hash_table_remove(tos->h, name);
+ StackObject *so = tos;
+ const char *key = name;
+ bool implicit_object;
+
+ assert(key);
+ if (!find_object_member(qiv, &so, &key, &implicit_object, errp)) {
+ if (implicit_object) {
+ if (!qiv->empty_qdict) {
+ qiv->empty_qdict = qdict_new();
+ }
+ return QOBJECT(qiv->empty_qdict);
+ } else {
+ return NULL;
+ }
+ }
+ ret = qdict_get(qobject_to(QDict, so->obj), key);
+ if (so->h && consume && ret) {
+ bool removed = g_hash_table_remove(so->h, key);
assert(removed);
}
} else {
@@ -190,9 +336,10 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
const char *name,
bool consume, Error **errp)
{
- QObject *obj = qobject_input_try_get_object(qiv, name, consume);
+ ERRP_GUARD();
+ QObject *obj = qobject_input_try_get_object(qiv, name, consume, errp);
- if (!obj) {
+ if (!obj && !*errp) {
error_setg(errp, QERR_MISSING_PARAMETER, full_name(qiv, name));
}
return obj;
@@ -764,13 +911,16 @@ static bool qobject_input_type_size_keyval(Visitor *v, const char *name,
static void qobject_input_optional(Visitor *v, const char *name, bool *present)
{
QObjectInputVisitor *qiv = to_qiv(v);
- QObject *qobj = qobject_input_try_get_object(qiv, name, false);
+ Error *local_err = NULL;
+ QObject *qobj = qobject_input_try_get_object(qiv, name, false, &local_err);
- if (!qobj) {
+ /* If there was an error, let the caller try and run into the error */
+ if (!qobj && !local_err) {
*present = false;
return;
}
+ error_free(local_err);
*present = true;
}
@@ -785,6 +935,7 @@ static void qobject_input_free(Visitor *v)
qobject_input_stack_object_free(tos);
}
+ qobject_unref(qiv->empty_qdict);
qobject_unref(qiv->root);
if (qiv->errname) {
g_string_free(qiv->errname, TRUE);
--
2.28.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 5/6] qapi: Add support for aliases
2020-11-12 17:28 [PATCH 0/6] qapi: Add support for aliases Kevin Wolf
` (3 preceding siblings ...)
2020-11-12 17:28 ` [PATCH 4/6] qapi: Apply aliases " Kevin Wolf
@ 2020-11-12 17:28 ` Kevin Wolf
2020-11-12 18:34 ` Eric Blake
` (2 more replies)
2020-11-12 17:28 ` [PATCH 6/6] tests/qapi-schema: Test cases " Kevin Wolf
` (2 subsequent siblings)
7 siblings, 3 replies; 32+ messages in thread
From: Kevin Wolf @ 2020-11-12 17:28 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow, armbru
Introduce alias definitions for object types (structs and unions). This
allows using the same QAPI type and visitor for many syntax variations
that exist in the external representation, like between QMP and the
command line. It also provides a new tool for evolving the schema while
maintaining backwards compatibility during a deprecation period.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
docs/devel/qapi-code-gen.txt | 37 +++++++++++++++++++++++---
docs/sphinx/qapidoc.py | 2 +-
scripts/qapi/expr.py | 34 +++++++++++++++++++++--
scripts/qapi/schema.py | 27 +++++++++++++++----
scripts/qapi/types.py | 4 ++-
scripts/qapi/visit.py | 33 ++++++++++++++++++++---
tests/qapi-schema/test-qapi.py | 7 ++++-
tests/qapi-schema/double-type.err | 2 +-
tests/qapi-schema/unknown-expr-key.err | 2 +-
9 files changed, 130 insertions(+), 18 deletions(-)
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 6906a06ad2..6da14d5275 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -231,7 +231,8 @@ Syntax:
'data': MEMBERS,
'*base': STRING,
'*if': COND,
- '*features': FEATURES }
+ '*features': FEATURES,
+ '*aliases': ALIASES }
MEMBERS = { MEMBER, ... }
MEMBER = STRING : TYPE-REF
| STRING : { 'type': TYPE-REF,
@@ -286,13 +287,15 @@ Syntax:
UNION = { 'union': STRING,
'data': BRANCHES,
'*if': COND,
- '*features': FEATURES }
+ '*features': FEATURES,
+ '*aliases': ALIASES }
| { 'union': STRING,
'data': BRANCHES,
'base': ( MEMBERS | STRING ),
'discriminator': STRING,
'*if': COND,
- '*features': FEATURES }
+ '*features': FEATURES,
+ '*aliases': ALIASES }
BRANCHES = { BRANCH, ... }
BRANCH = STRING : TYPE-REF
| STRING : { 'type': TYPE-REF, '*if': COND }
@@ -837,6 +840,34 @@ shows a conditional entity only when the condition is satisfied in
this particular build.
+=== Aliases ===
+
+Object types, including structs and unions, can contain alias
+definitions.
+
+Aliases define alternative member names that may be used in the
+external representation to provide a value for a member in the same
+object or in a nested object.
+
+Syntax:
+ ALIAS = { '*alias': STRING,
+ 'source': [ STRING, ... ] }
+
+'source' is a list of member names representing the path to an object
+member, starting from the type where the alias definition is
+specified. It may refer to another alias name. It is allowed to use
+a path that doesn't necessarily match an existing member in every
+variant or even at all; in this case, the alias remains unused.
+
+If 'alias' is present, then the single member referred to by 'source'
+is made accessible with the name given in 'alias' in the type where
+the alias definition is specified.
+
+If 'alias' is not present, then all members in the object referred to
+by 'source' are made accessible in the type where the alias definition
+is specified with the same name as they have in 'source'.
+
+
=== Documentation comments ===
A multi-line comment that starts and ends with a '##' line is a
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index e03abcbb95..6c94c01148 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -310,7 +310,7 @@ class QAPISchemaGenRSTVisitor(QAPISchemaVisitor):
+ self._nodes_for_if_section(ifcond))
def visit_object_type(self, name, info, ifcond, features,
- base, members, variants):
+ base, members, variants, aliases):
doc = self._cur_doc
if base and base.is_implicit():
base = None
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 2fcaaa2497..21fded529b 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -198,6 +198,32 @@ def check_features(features, info):
check_if(f, info, source)
+def check_aliases(aliases, info):
+ if aliases is None:
+ return
+ if not isinstance(aliases, list):
+ raise QAPISemError(info, "'aliases' must be an array")
+ for a in aliases:
+ if not isinstance(a, dict):
+ raise QAPISemError(info, "'aliases' entries must be objects")
+ check_keys(a, info, "alias", ['source'], ['alias'])
+
+ if 'alias' in a:
+ source = "alias member 'alias'"
+ check_name_is_str(a['alias'], info, source)
+ check_name_str(a['alias'], info, source)
+
+ if not isinstance(a['source'], list):
+ raise QAPISemError(info, "'source' must be an array")
+ if not a['source']:
+ raise QAPISemError(info, "'source' must not be empty")
+
+ source = "element of alias member 'source'"
+ for s in a['source']:
+ check_name_is_str(s, info, source)
+ check_name_str(s, info, source)
+
+
def check_enum(expr, info):
name = expr['enum']
members = expr['data']
@@ -228,6 +254,7 @@ def check_struct(expr, info):
check_type(members, info, "'data'", allow_dict=name)
check_type(expr.get('base'), info, "'base'")
+ check_aliases(expr.get('aliases'), info)
def check_union(expr, info):
@@ -245,6 +272,8 @@ def check_union(expr, info):
raise QAPISemError(info, "'discriminator' requires 'base'")
check_name_is_str(discriminator, info, "'discriminator'")
+ check_aliases(expr.get('aliases'), info)
+
for (key, value) in members.items():
source = "'data' member '%s'" % key
check_name_str(key, info, source)
@@ -331,7 +360,7 @@ def check_exprs(exprs):
elif meta == 'union':
check_keys(expr, info, meta,
['union', 'data'],
- ['base', 'discriminator', 'if', 'features'])
+ ['base', 'discriminator', 'if', 'features', 'aliases'])
normalize_members(expr.get('base'))
normalize_members(expr['data'])
check_union(expr, info)
@@ -342,7 +371,8 @@ def check_exprs(exprs):
check_alternate(expr, info)
elif meta == 'struct':
check_keys(expr, info, meta,
- ['struct', 'data'], ['base', 'if', 'features'])
+ ['struct', 'data'],
+ ['base', 'if', 'features', 'aliases'])
normalize_members(expr['data'])
check_struct(expr, info)
elif meta == 'command':
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee..5daa137163 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -117,7 +117,7 @@ class QAPISchemaVisitor:
pass
def visit_object_type(self, name, info, ifcond, features,
- base, members, variants):
+ base, members, variants, aliases):
pass
def visit_object_type_flat(self, name, info, ifcond, features,
@@ -331,9 +331,16 @@ class QAPISchemaArrayType(QAPISchemaType):
return "%s type ['%s']" % (self.meta, self._element_type_name)
+class QAPISchemaAlias:
+ def __init__(self, alias, source):
+ assert source
+ self.alias = alias
+ self.source = source
+
+
class QAPISchemaObjectType(QAPISchemaType):
def __init__(self, name, info, doc, ifcond, features,
- base, local_members, variants):
+ base, local_members, variants, aliases=None):
# struct has local_members, optional base, and no variants
# flat union has base, variants, and no local_members
# simple union has local_members, variants, and no base
@@ -351,6 +358,7 @@ class QAPISchemaObjectType(QAPISchemaType):
self.local_members = local_members
self.variants = variants
self.members = None
+ self.aliases = aliases or []
def check(self, schema):
# This calls another type T's .check() exactly when the C
@@ -443,7 +451,7 @@ class QAPISchemaObjectType(QAPISchemaType):
super().visit(visitor)
visitor.visit_object_type(
self.name, self.info, self.ifcond, self.features,
- self.base, self.local_members, self.variants)
+ self.base, self.local_members, self.variants, self.aliases)
visitor.visit_object_type_flat(
self.name, self.info, self.ifcond, self.features,
self.members, self.variants)
@@ -934,6 +942,12 @@ class QAPISchema:
return [QAPISchemaFeature(f['name'], info, f.get('if'))
for f in features]
+ def _make_aliases(self, aliases):
+ if aliases is None:
+ return []
+ return [QAPISchemaAlias(a.get('alias'), a['source'])
+ for a in aliases]
+
def _make_enum_members(self, values, info):
return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
for v in values]
@@ -1008,11 +1022,12 @@ class QAPISchema:
base = expr.get('base')
data = expr['data']
ifcond = expr.get('if')
+ aliases = self._make_aliases(expr.get('aliases'))
features = self._make_features(expr.get('features'), info)
self._def_entity(QAPISchemaObjectType(
name, info, doc, ifcond, features, base,
self._make_members(data, info),
- None))
+ None, aliases))
def _make_variant(self, case, typ, ifcond, info):
return QAPISchemaVariant(case, info, typ, ifcond)
@@ -1031,6 +1046,7 @@ class QAPISchema:
data = expr['data']
base = expr.get('base')
ifcond = expr.get('if')
+ aliases = self._make_aliases(expr.get('aliases'))
features = self._make_features(expr.get('features'), info)
tag_name = expr.get('discriminator')
tag_member = None
@@ -1055,7 +1071,8 @@ class QAPISchema:
QAPISchemaObjectType(name, info, doc, ifcond, features,
base, members,
QAPISchemaVariants(
- tag_name, info, tag_member, variants)))
+ tag_name, info, tag_member, variants),
+ aliases))
def _def_alternate_type(self, expr, info, doc):
name = expr['alternate']
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2b4916cdaa..e870bebb7e 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -25,6 +25,7 @@ from .common import (
from .gen import QAPISchemaModularCVisitor, ifcontext
from .schema import (
QAPISchema,
+ QAPISchemaAlias,
QAPISchemaEnumMember,
QAPISchemaFeature,
QAPISchemaObjectType,
@@ -332,7 +333,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
features: List[QAPISchemaFeature],
base: Optional[QAPISchemaObjectType],
members: List[QAPISchemaObjectTypeMember],
- variants: Optional[QAPISchemaVariants]) -> None:
+ variants: Optional[QAPISchemaVariants],
+ aliases: List[QAPISchemaAlias]) -> None:
# Nothing to do for the special empty builtin
if name == 'q_empty':
return
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 339f152152..a35921ef2c 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -26,6 +26,7 @@ from .common import (
from .gen import QAPISchemaModularCVisitor, ifcontext
from .schema import (
QAPISchema,
+ QAPISchemaAlias,
QAPISchemaEnumMember,
QAPISchemaEnumType,
QAPISchemaFeature,
@@ -60,7 +61,8 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
def gen_visit_object_members(name: str,
base: Optional[QAPISchemaObjectType],
members: List[QAPISchemaObjectTypeMember],
- variants: Optional[QAPISchemaVariants]) -> str:
+ variants: Optional[QAPISchemaVariants],
+ aliases: List[QAPISchemaAlias]) -> str:
ret = mcgen('''
bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
@@ -68,6 +70,25 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
''',
c_name=c_name(name))
+ if aliases:
+ ret += mcgen('''
+ visit_start_alias_scope(v);
+''')
+
+ for a in aliases:
+ if a.alias:
+ alias = '"%s"' % a.alias
+ else:
+ alias = "NULL"
+
+ source_list = ", ".join('"%s"' % x for x in a.source)
+ source = "(const char * []) { %s, NULL }" % source_list
+
+ ret += mcgen('''
+ visit_define_alias(v, %(alias)s, %(source)s);
+''',
+ alias=alias, source=source)
+
if base:
ret += mcgen('''
if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
@@ -133,6 +154,11 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
}
''')
+ if aliases:
+ ret += mcgen('''
+ visit_end_alias_scope(v);
+''')
+
ret += mcgen('''
return true;
}
@@ -361,14 +387,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
features: List[QAPISchemaFeature],
base: Optional[QAPISchemaObjectType],
members: List[QAPISchemaObjectTypeMember],
- variants: Optional[QAPISchemaVariants]) -> None:
+ variants: Optional[QAPISchemaVariants],
+ aliases: List[QAPISchemaAlias]) -> None:
# Nothing to do for the special empty builtin
if name == 'q_empty':
return
with ifcontext(ifcond, self._genh, self._genc):
self._genh.add(gen_visit_members_decl(name))
self._genc.add(gen_visit_object_members(name, base,
- members, variants))
+ members, variants, aliases))
# TODO Worth changing the visitor signature, so we could
# directly use rather than repeat type.is_implicit()?
if not name.startswith('q_'):
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index e8db9d09d9..adf8bda89a 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -47,7 +47,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
self._print_if(ifcond)
def visit_object_type(self, name, info, ifcond, features,
- base, members, variants):
+ base, members, variants, aliases):
print('object %s' % name)
if base:
print(' base %s' % base.name)
@@ -56,6 +56,11 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
% (m.name, m.type.name, m.optional))
self._print_if(m.ifcond, 8)
self._print_features(m.features, indent=8)
+ for a in aliases:
+ if a.alias:
+ print(' alias %s -> %s' % (a.alias, '/'.join(a.source)))
+ else:
+ print(' alias * -> %s/*' % '/'.join(a.source))
self._print_variants(variants)
self._print_if(ifcond)
self._print_features(features)
diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err
index 71fc4dbb52..5d25d7623c 100644
--- a/tests/qapi-schema/double-type.err
+++ b/tests/qapi-schema/double-type.err
@@ -1,3 +1,3 @@
double-type.json: In struct 'bar':
double-type.json:2: struct has unknown key 'command'
-Valid keys are 'base', 'data', 'features', 'if', 'struct'.
+Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'.
diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err
index c5f395bf79..7429d1ff03 100644
--- a/tests/qapi-schema/unknown-expr-key.err
+++ b/tests/qapi-schema/unknown-expr-key.err
@@ -1,3 +1,3 @@
unknown-expr-key.json: In struct 'bar':
unknown-expr-key.json:2: struct has unknown keys 'bogus', 'phony'
-Valid keys are 'base', 'data', 'features', 'if', 'struct'.
+Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'.
--
2.28.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 6/6] tests/qapi-schema: Test cases for aliases
2020-11-12 17:28 [PATCH 0/6] qapi: Add support for aliases Kevin Wolf
` (4 preceding siblings ...)
2020-11-12 17:28 ` [PATCH 5/6] qapi: Add support for aliases Kevin Wolf
@ 2020-11-12 17:28 ` Kevin Wolf
2021-02-10 13:09 ` Markus Armbruster
2020-12-04 9:46 ` [PATCH 0/6] qapi: Add support " Kevin Wolf
2021-02-10 14:38 ` Markus Armbruster
7 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2020-11-12 17:28 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow, armbru
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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-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
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 | 8 +++++
tests/qapi-schema/qapi-schema-test.json | 24 +++++++++++++++
tests/qapi-schema/qapi-schema-test.out | 29 +++++++++++++++++++
27 files changed, 102 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-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-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/qapi-schema/alias-bad-type.err b/tests/qapi-schema/alias-bad-type.err
new file mode 100644
index 0000000000..a982d380b8
--- /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' entries 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..1cfe8a6aa5
--- /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: alias 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..4ae22c2221
--- /dev/null
+++ b/tests/qapi-schema/alias-missing-source.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'alias': '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..83b9fe0b65
--- /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 'alias' 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..6d993be332
--- /dev/null
+++ b/tests/qapi-schema/alias-name-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'alias': ['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-source-bad-type.err b/tests/qapi-schema/alias-source-bad-type.err
new file mode 100644
index 0000000000..e3b949ff77
--- /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: '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..0099a6439e
--- /dev/null
+++ b/tests/qapi-schema/alias-source-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'alias': '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..44d219e352
--- /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: element 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..28cb1c37c5
--- /dev/null
+++ b/tests/qapi-schema/alias-source-elem-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'alias': '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..f7687e404c
--- /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: '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..9d2d2f109f
--- /dev/null
+++ b/tests/qapi-schema/alias-source-empty.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'alias': '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-unknown-key.err b/tests/qapi-schema/alias-unknown-key.err
new file mode 100644
index 0000000000..d4dc5e4611
--- /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: alias has unknown key 'known'
+Valid keys are 'alias', '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..444f80ca30
--- /dev/null
+++ b/tests/qapi-schema/alias-unknown-key.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'alias': '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 304ef939bd..710cd60b61 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -3,6 +3,14 @@ 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-source-bad-type.json',
+ 'alias-source-elem-bad-type.json',
+ 'alias-source-empty.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 63f92adf68..cc2497b2a2 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -325,3 +325,27 @@
{ 'event': 'TEST-EVENT-FEATURES1',
'features': [ 'deprecated' ] }
+
+# test 'aliases'
+
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [] }
+{ 'struct': 'AliasStruct1',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'alias': 'bar', 'source': ['foo'] } ] }
+{ 'struct': 'AliasStruct2',
+ 'data': { 'nested': 'AliasStruct1' },
+ 'aliases': [ { 'alias': 'bar', 'source': ['nested', 'foo'] } ] }
+{ 'struct': 'AliasStruct3',
+ 'data': { 'nested': 'AliasStruct1' },
+ 'aliases': [ { 'source': ['nested'] } ] }
+
+{ 'union': 'AliasFlatUnion',
+ 'base': { 'tag': 'FeatureEnum1' },
+ 'discriminator': 'tag',
+ 'data': { 'eins': 'FeatureStruct1' },
+ 'aliases': [ { 'alias': 'bar', 'source': ['foo'] } ] }
+{ 'union': 'AliasSimpleUnion',
+ 'data': { 'eins': 'AliasStruct1' },
+ 'aliases': [ { 'source': ['data'] } ] }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 8868ca0dca..8ed88a257d 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -443,6 +443,35 @@ command test-command-cond-features3 None -> None
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 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/*
+ tag type
+ case eins: q_obj_AliasStruct1-wrapper
module include/sub-module.json
include sub-sub-module.json
object SecondArrayRef
--
2.28.0
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] qapi: Add support for aliases
2020-11-12 17:28 ` [PATCH 5/6] qapi: Add support for aliases Kevin Wolf
@ 2020-11-12 18:34 ` Eric Blake
2020-11-13 9:46 ` Kevin Wolf
2021-02-10 9:17 ` Markus Armbruster
2021-02-10 14:29 ` Markus Armbruster
2 siblings, 1 reply; 32+ messages in thread
From: Eric Blake @ 2020-11-12 18:34 UTC (permalink / raw)
To: Kevin Wolf, qemu-devel; +Cc: jsnow, armbru
On 11/12/20 11:28 AM, Kevin Wolf wrote:
> Introduce alias definitions for object types (structs and unions). This
> allows using the same QAPI type and visitor for many syntax variations
> that exist in the external representation, like between QMP and the
> command line. It also provides a new tool for evolving the schema while
> maintaining backwards compatibility during a deprecation period.
Cool! Obvious followup patch series: deprecate all QAPI members spelled
with _ by making them aliases of new members spelled with -, so that we
can finally have consistent spellings.
> +=== Aliases ===
> +
> +Object types, including structs and unions, can contain alias
> +definitions.
> +
> +Aliases define alternative member names that may be used in the
> +external representation to provide a value for a member in the same
> +object or in a nested object.
> +
> +Syntax:
> + ALIAS = { '*alias': STRING,
> + 'source': [ STRING, ... ] }
> +
> +'source' is a list of member names representing the path to an object
> +member, starting from the type where the alias definition is
> +specified. It may refer to another alias name. It is allowed to use
> +a path that doesn't necessarily match an existing member in every
> +variant or even at all; in this case, the alias remains unused.
> +
> +If 'alias' is present, then the single member referred to by 'source'
> +is made accessible with the name given in 'alias' in the type where
> +the alias definition is specified.
> +
> +If 'alias' is not present, then all members in the object referred to
> +by 'source' are made accessible in the type where the alias definition
> +is specified with the same name as they have in 'source'.
Is it worth an example of how to use this?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] qapi: Add support for aliases
2020-11-12 18:34 ` Eric Blake
@ 2020-11-13 9:46 ` Kevin Wolf
2020-11-20 14:41 ` Peter Krempa
0 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2020-11-13 9:46 UTC (permalink / raw)
To: Eric Blake; +Cc: libvir-list, jsnow, qemu-devel, armbru
Am 12.11.2020 um 19:34 hat Eric Blake geschrieben:
> On 11/12/20 11:28 AM, Kevin Wolf wrote:
> > Introduce alias definitions for object types (structs and unions). This
> > allows using the same QAPI type and visitor for many syntax variations
> > that exist in the external representation, like between QMP and the
> > command line. It also provides a new tool for evolving the schema while
> > maintaining backwards compatibility during a deprecation period.
>
> Cool! Obvious followup patch series: deprecate all QAPI members spelled
> with _ by making them aliases of new members spelled with -, so that we
> can finally have consistent spellings.
Ah, that's a nice one, too. I didn't even think of it. Another one I'd
like to see is deprecation of SocketAddressLegacy.
There is one part missing in this series that we would first need to
address before we can actually use it to evolve parts of the schema that
are visible in QMP: Exposing aliases in introspection and expressing
that the original name of something is deprecated, but the alias will
stay around (and probably also deprecating an alias without the original
name or other aliases).
If we can easily figure out a way to express this that everyone agrees
with, I'm happy to include it in this series. Otherwise, since the first
user is the command line and not QMP, I'd leave that for the future.
For example, imagine we have an option 'foo' with a new alias 'bar'. If
we just directly expose the alias rule (which would be the simplest
solution from the QEMU perspective), management will check if the alias
exists before accessing 'bar'. But in the final state, we remove 'foo'
and 'bar' is not an alias any more, so the introspection for 'bar' would
change. Is this desirable?
On the other hand, we can't specify both as normal options because you
must set (at most) one of them, but not both. Also exposing things as
normal options becomes hard with wildcard aliases (mapping all fields
from a nested struct), especially if unions are involved where some
options exist in one or two variants, but not in others.
Given this, I think just exposing the alias rules and letting the
management tool check both alternatives - if the alias rule or the
future option exists - might actually still be the least bad option.
Hmm, I guess I should CC libvirt for this discussion, actually. :-)
> > +=== Aliases ===
> > +
> > +Object types, including structs and unions, can contain alias
> > +definitions.
> > +
> > +Aliases define alternative member names that may be used in the
> > +external representation to provide a value for a member in the same
> > +object or in a nested object.
> > +
> > +Syntax:
> > + ALIAS = { '*alias': STRING,
> > + 'source': [ STRING, ... ] }
> > +
> > +'source' is a list of member names representing the path to an object
> > +member, starting from the type where the alias definition is
> > +specified. It may refer to another alias name. It is allowed to use
> > +a path that doesn't necessarily match an existing member in every
> > +variant or even at all; in this case, the alias remains unused.
> > +
> > +If 'alias' is present, then the single member referred to by 'source'
> > +is made accessible with the name given in 'alias' in the type where
> > +the alias definition is specified.
> > +
> > +If 'alias' is not present, then all members in the object referred to
> > +by 'source' are made accessible in the type where the alias definition
> > +is specified with the same name as they have in 'source'.
>
> Is it worth an example of how to use this?
Yes, I should have included an example. Or actually, probably one
example for aliasing a single field and another one for a wildcard alias
that maps all fields in a struct.
Kevin
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] qapi: Add support for aliases
2020-11-13 9:46 ` Kevin Wolf
@ 2020-11-20 14:41 ` Peter Krempa
2020-11-20 15:06 ` Daniel P. Berrangé
0 siblings, 1 reply; 32+ messages in thread
From: Peter Krempa @ 2020-11-20 14:41 UTC (permalink / raw)
To: Kevin Wolf; +Cc: libvir-list, jsnow, qemu-devel
On Fri, Nov 13, 2020 at 10:46:02 +0100, Kevin Wolf wrote:
> Am 12.11.2020 um 19:34 hat Eric Blake geschrieben:
> > On 11/12/20 11:28 AM, Kevin Wolf wrote:
> > > Introduce alias definitions for object types (structs and unions). This
> > > allows using the same QAPI type and visitor for many syntax variations
> > > that exist in the external representation, like between QMP and the
> > > command line. It also provides a new tool for evolving the schema while
> > > maintaining backwards compatibility during a deprecation period.
> >
> > Cool! Obvious followup patch series: deprecate all QAPI members spelled
> > with _ by making them aliases of new members spelled with -, so that we
> > can finally have consistent spellings.
>
> Ah, that's a nice one, too. I didn't even think of it. Another one I'd
> like to see is deprecation of SocketAddressLegacy.
>
> There is one part missing in this series that we would first need to
> address before we can actually use it to evolve parts of the schema that
> are visible in QMP: Exposing aliases in introspection and expressing
> that the original name of something is deprecated, but the alias will
> stay around (and probably also deprecating an alias without the original
> name or other aliases).
>
> If we can easily figure out a way to express this that everyone agrees
> with, I'm happy to include it in this series. Otherwise, since the first
> user is the command line and not QMP, I'd leave that for the future.
>
> For example, imagine we have an option 'foo' with a new alias 'bar'. If
> we just directly expose the alias rule (which would be the simplest
> solution from the QEMU perspective), management will check if the alias
> exists before accessing 'bar'. But in the final state, we remove 'foo'
> and 'bar' is not an alias any more, so the introspection for 'bar' would
> change. Is this desirable?
>
> On the other hand, we can't specify both as normal options because you
> must set (at most) one of them, but not both. Also exposing things as
> normal options becomes hard with wildcard aliases (mapping all fields
> from a nested struct), especially if unions are involved where some
> options exist in one or two variants, but not in others.
>
> Given this, I think just exposing the alias rules and letting the
> management tool check both alternatives - if the alias rule or the
> future option exists - might actually still be the least bad option.
>
> Hmm, I guess I should CC libvirt for this discussion, actually. :-)
I can see how that simplifies things for qemu in the long run, but to be
honest, if you then deprecate the old name, libvirt will need to add a
translation table for it. Either explicit by detecting the new name and
adapting the code or by adding a lookup table or something. This would
be needed as if you then remove the alias itself we'd no longer be able
to use it, so I'm not entirely a fan of it, especially the deprecation
bit.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] qapi: Add support for aliases
2020-11-20 14:41 ` Peter Krempa
@ 2020-11-20 15:06 ` Daniel P. Berrangé
0 siblings, 0 replies; 32+ messages in thread
From: Daniel P. Berrangé @ 2020-11-20 15:06 UTC (permalink / raw)
To: Peter Krempa; +Cc: Kevin Wolf, libvir-list, jsnow, qemu-devel
On Fri, Nov 20, 2020 at 03:41:54PM +0100, Peter Krempa wrote:
> On Fri, Nov 13, 2020 at 10:46:02 +0100, Kevin Wolf wrote:
> > Am 12.11.2020 um 19:34 hat Eric Blake geschrieben:
> > > On 11/12/20 11:28 AM, Kevin Wolf wrote:
> > > > Introduce alias definitions for object types (structs and unions). This
> > > > allows using the same QAPI type and visitor for many syntax variations
> > > > that exist in the external representation, like between QMP and the
> > > > command line. It also provides a new tool for evolving the schema while
> > > > maintaining backwards compatibility during a deprecation period.
> > >
> > > Cool! Obvious followup patch series: deprecate all QAPI members spelled
> > > with _ by making them aliases of new members spelled with -, so that we
> > > can finally have consistent spellings.
> >
> > Ah, that's a nice one, too. I didn't even think of it. Another one I'd
> > like to see is deprecation of SocketAddressLegacy.
> >
> > There is one part missing in this series that we would first need to
> > address before we can actually use it to evolve parts of the schema that
> > are visible in QMP: Exposing aliases in introspection and expressing
> > that the original name of something is deprecated, but the alias will
> > stay around (and probably also deprecating an alias without the original
> > name or other aliases).
> >
> > If we can easily figure out a way to express this that everyone agrees
> > with, I'm happy to include it in this series. Otherwise, since the first
> > user is the command line and not QMP, I'd leave that for the future.
> >
> > For example, imagine we have an option 'foo' with a new alias 'bar'. If
> > we just directly expose the alias rule (which would be the simplest
> > solution from the QEMU perspective), management will check if the alias
> > exists before accessing 'bar'. But in the final state, we remove 'foo'
> > and 'bar' is not an alias any more, so the introspection for 'bar' would
> > change. Is this desirable?
> >
> > On the other hand, we can't specify both as normal options because you
> > must set (at most) one of them, but not both. Also exposing things as
> > normal options becomes hard with wildcard aliases (mapping all fields
> > from a nested struct), especially if unions are involved where some
> > options exist in one or two variants, but not in others.
> >
> > Given this, I think just exposing the alias rules and letting the
> > management tool check both alternatives - if the alias rule or the
> > future option exists - might actually still be the least bad option.
> >
> > Hmm, I guess I should CC libvirt for this discussion, actually. :-)
>
> I can see how that simplifies things for qemu in the long run, but to be
> honest, if you then deprecate the old name, libvirt will need to add a
> translation table for it. Either explicit by detecting the new name and
> adapting the code or by adding a lookup table or something. This would
> be needed as if you then remove the alias itself we'd no longer be able
> to use it, so I'm not entirely a fan of it, especially the deprecation
> bit.
To be fair, a key part of libvirt's value add is that it provides the
stable API to applications, insulating them from hypervisor changes.
This enables QEMU to make incompatible changes in a reasonable timeframe,
without breaking the end applications.
Of course there is a trade off here because every time QEMU changes,
libvirt gets to add back compat code to its maint burden, but the
premise is that this is still cheaper overall.
We don't want QEMU to gratuitously break things for no reason, but
if there's a compelling reason to change QEMU, libvirt is there to pick
up the pieces to protect applications.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/6] qapi: Add support for aliases
2020-11-12 17:28 [PATCH 0/6] qapi: Add support for aliases Kevin Wolf
` (5 preceding siblings ...)
2020-11-12 17:28 ` [PATCH 6/6] tests/qapi-schema: Test cases " Kevin Wolf
@ 2020-12-04 9:46 ` Kevin Wolf
2021-02-10 14:38 ` Markus Armbruster
7 siblings, 0 replies; 32+ messages in thread
From: Kevin Wolf @ 2020-12-04 9:46 UTC (permalink / raw)
To: qemu-devel; +Cc: jsnow, armbru
Am 12.11.2020 um 18:28 hat Kevin Wolf geschrieben:
> This series introduces alias definitions for QAPI object types (structs
> and unions).
>
> This allows using the same QAPI type and visitor even when the syntax
> has some variations between different external interfaces such as QMP
> and the command line.
>
> It also provides a new tool for evolving the schema while maintaining
> backwards compatibility (possibly during a deprecation period).
>
> The first user is intended to be a QAPIfied -chardev command line
> option, for which I'll send a separate series. A git tag is available
> that contains both this series and the chardev changes that make use of
> it:
>
> https://repo.or.cz/qemu/kevin.git qapi-alias-chardev-v1
>
> Kevin Wolf (6):
> qapi: Add interfaces for alias support to Visitor
> qapi: Remember alias definitions in qobject-input-visitor
> qapi: Simplify full_name_nth() in qobject-input-visitor
> qapi: Apply aliases in qobject-input-visitor
> qapi: Add support for aliases
> tests/qapi-schema: Test cases for aliases
Ping.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/6] qapi: Add interfaces for alias support to Visitor
2020-11-12 17:28 ` [PATCH 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
@ 2021-01-26 15:59 ` Markus Armbruster
2021-01-27 12:51 ` Markus Armbruster
1 sibling, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2021-01-26 15:59 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> This adds functions to the Visitor interface that can be used to define
> aliases and alias scopes.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/qapi/visitor-impl.h | 12 ++++++++++++
> include/qapi/visitor.h | 37 +++++++++++++++++++++++++++++++++++++
> qapi/qapi-visit-core.c | 21 +++++++++++++++++++++
> 3 files changed, 70 insertions(+)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 7362c043be..e30da2599c 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -113,6 +113,18 @@ struct Visitor
> The core takes care of the return type in the public interface. */
> void (*optional)(Visitor *v, const char *name, bool *present);
>
> + /*
> + * Optional; intended for input visitors. If not given, aliases are
> + * ignored.
> + */
> + void (*define_alias)(Visitor *v, const char *alias, const char **source);
> +
> + /* Must be set if define_alias is set */
> + void (*start_alias_scope)(Visitor *v);
> +
> + /* Must be set if define_alias is set */
> + void (*end_alias_scope)(Visitor *v);
> +
> /* Must be set */
> VisitorType type;
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index ebc19ede7f..9bdc0ee03d 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -459,6 +459,43 @@ void visit_end_alternate(Visitor *v, void **obj);
> */
> bool visit_optional(Visitor *v, const char *name, bool *present);
>
> +/*
> + * Defines a new alias rule.
> + *
> + * If @alias is non-NULL, the member named @alias in the external
> + * representation of the current struct is defined as an alias for the
> + * member described by @source.
> + *
> + * If @alias is NULL, all members of the struct described by @source are
> + * considered to have alias members with the same key in the current
> + * struct.
> + *
> + * @source is a NULL-terminated array of names that describe the path to
> + * a member, starting from the currently visited struct.
> + *
> + * The alias stays valid until the current alias scope ends.
> + * visit_start/end_struct() implicitly start/end an alias scope.
> + * Additionally, visit_start/end_alias_scope() can be used to explicitly
> + * create a nested alias scope.
> + */
> +void visit_define_alias(Visitor *v, const char *alias, const char **source);
> +
> +/*
> + * Begins an explicit alias scope.
> + *
> + * Alias definitions after here will only stay valid until the
> + * corresponding visit_end_alias_scope() is called.
> + */
> +void visit_start_alias_scope(Visitor *v);
> +
> +/*
> + * Ends an explicit alias scope.
> + *
> + * Alias definitions between the correspoding visit_start_alias_scope()
> + * call and here go out of scope and won't apply in later code any more.
> + */
> +void visit_end_alias_scope(Visitor *v);
> +
> /*
> * Visit an enum value.
> *
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 7e5f40e7f0..719a9f5da2 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -135,6 +135,27 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
> return *present;
> }
>
> +void visit_define_alias(Visitor *v, const char *alias, const char **source)
> +{
> + if (v->define_alias) {
> + v->define_alias(v, alias, source);
> + }
> +}
> +
> +void visit_start_alias_scope(Visitor *v)
> +{
> + if (v->start_alias_scope) {
> + v->start_alias_scope(v);
> + }
> +}
> +
> +void visit_end_alias_scope(Visitor *v)
> +{
> + if (v->end_alias_scope) {
> + v->end_alias_scope(v);
> + }
> +}
> +
> bool visit_is_input(Visitor *v)
> {
> return v->type == VISITOR_INPUT;
The code is trivial and neat, the contracts look fine. What I'm missing
is "why do we want this", and "how should it be used". The cover letter
gives me a (somewhat vague) idea on the former, but on the latter, I can
only guess. I trust it'll become clear later in this series.
I think updating the big comment in visitor.h for aliases would help.
Let's postpone it until I've seen the rest of the series.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/6] qapi: Add interfaces for alias support to Visitor
2020-11-12 17:28 ` [PATCH 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
2021-01-26 15:59 ` Markus Armbruster
@ 2021-01-27 12:51 ` Markus Armbruster
2021-01-27 20:31 ` Kevin Wolf
1 sibling, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2021-01-27 12:51 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> This adds functions to the Visitor interface that can be used to define
> aliases and alias scopes.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/qapi/visitor-impl.h | 12 ++++++++++++
> include/qapi/visitor.h | 37 +++++++++++++++++++++++++++++++++++++
> qapi/qapi-visit-core.c | 21 +++++++++++++++++++++
> 3 files changed, 70 insertions(+)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 7362c043be..e30da2599c 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -113,6 +113,18 @@ struct Visitor
> The core takes care of the return type in the public interface. */
> void (*optional)(Visitor *v, const char *name, bool *present);
>
> + /*
> + * Optional; intended for input visitors. If not given, aliases are
> + * ignored.
> + */
> + void (*define_alias)(Visitor *v, const char *alias, const char **source);
> +
> + /* Must be set if define_alias is set */
> + void (*start_alias_scope)(Visitor *v);
> +
> + /* Must be set if define_alias is set */
> + void (*end_alias_scope)(Visitor *v);
> +
> /* Must be set */
> VisitorType type;
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index ebc19ede7f..9bdc0ee03d 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -459,6 +459,43 @@ void visit_end_alternate(Visitor *v, void **obj);
> */
> bool visit_optional(Visitor *v, const char *name, bool *present);
>
> +/*
> + * Defines a new alias rule.
> + *
> + * If @alias is non-NULL, the member named @alias in the external
> + * representation of the current struct is defined as an alias for the
Terminology: the big comment uses "object". See also the FIXME in
visit_start_struct()'s contract.
> + * member described by @source.
> + *
> + * If @alias is NULL, all members of the struct described by @source are
> + * considered to have alias members with the same key in the current
> + * struct.
Define "the current struct". I believe it's the object being visited.
What happens if we're currently visiting something other than an object,
i.e. the root of a tree, or a list?
> + *
> + * @source is a NULL-terminated array of names that describe the path to
> + * a member, starting from the currently visited struct.
I'm afraid "describe the path to a member" is too vague. How?
I figure this is what you have in mind:
cur = the currently visited object
for s in source:
cur = the member of cur denoted by s
When @cur is indeed an object, then "the member denoted by @s" makes
sense: you must pass a name when visiting object members, and whatever
is visited with name @s is the member denoted by @s.
"Must pass a name" is documented in the big comment:
* The @name parameter of visit_type_FOO() describes the relation
* between this QAPI value and its parent container. When visiting
* the root of a tree, @name is ignored; when visiting a member of an
* object, @name is the key associated with the value; when visiting a
* member of a list, @name is NULL; and when visiting the member of an
* alternate, @name should equal the name used for visiting the
* alternate.
But what if @cur is a list? I guess that makes no sense. Say so
explicitly, please.
> + *
> + * The alias stays valid until the current alias scope ends.
> + * visit_start/end_struct() implicitly start/end an alias scope.
> + * Additionally, visit_start/end_alias_scope() can be used to explicitly
> + * create a nested alias scope.
> + */
> +void visit_define_alias(Visitor *v, const char *alias, const char **source);
> +
> +/*
> + * Begins an explicit alias scope.
> + *
> + * Alias definitions after here will only stay valid until the
> + * corresponding visit_end_alias_scope() is called.
> + */
> +void visit_start_alias_scope(Visitor *v);
> +
> +/*
> + * Ends an explicit alias scope.
> + *
> + * Alias definitions between the correspoding visit_start_alias_scope()
> + * call and here go out of scope and won't apply in later code any more.
> + */
> +void visit_end_alias_scope(Visitor *v);
> +
> /*
> * Visit an enum value.
> *
[...]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor
2020-11-12 17:28 ` [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor Kevin Wolf
@ 2021-01-27 13:06 ` Markus Armbruster
2021-01-27 20:59 ` Kevin Wolf
2021-02-09 12:57 ` Markus Armbruster
1 sibling, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2021-01-27 13:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel, armbru
Kevin Wolf <kwolf@redhat.com> writes:
> This makes qobject-input-visitor remember the currently valid aliases in
> each StackObject. It doesn't actually allow using the aliases yet.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++++++++++++
> 1 file changed, 115 insertions(+)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 23843b242e..a00ac32682 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -29,6 +29,29 @@
> #include "qemu/cutils.h"
> #include "qemu/option.h"
>
> +typedef struct InputVisitorAlias {
> + /* Alias name. NULL for any (wildcard alias). */
> + const char *alias;
> +
> + /*
> + * NULL terminated array representing a path.
> + * Must contain at least one non-NULL element if alias is not NULL.
What does .alias = NULL, .src[] empty mean?
The previous patch's contract for visit_define_alias():
/*
* Defines a new alias rule.
*
* If @alias is non-NULL, the member named @alias in the external
* representation of the current struct is defined as an alias for the
* member described by @source.
*
* If @alias is NULL, all members of the struct described by @source are
* considered to have alias members with the same key in the current
* struct.
*
* @source is a NULL-terminated array of names that describe the path to
* a member, starting from the currently visited struct.
*
* The alias stays valid until the current alias scope ends.
* visit_start/end_struct() implicitly start/end an alias scope.
* Additionally, visit_start/end_alias_scope() can be used to explicitly
* create a nested alias scope.
*/
If I read this correctly, then empty .src[] denotes "the current
struct", and therefore .alias = NULL, .src[] empty means "all members of
the current struct are considered to have alias members with the same
key in the current struct". Which is be a complicated way to accomplish
nothing.
Am I confused?
> + */
> + const char **src;
> +
> + /* StackObject in which the alias should be looked for */
> + struct StackObject *alias_so;
Clear as mud. Is it "the current struct"? If not, what else? Perhaps
an example would help me understand.
> +
> + /*
> + * The alias remains valid as long as the containing StackObject has
What's "the containing StackObject"? I guess it's the one that has this
InputVisitorAlias in .aliases.
> + * StackObject.alias_scope_nesting >= InputVisitorAlias.scope_nesting
> + * or until the whole StackObject is removed.
> + */
> + int scope_nesting;
> +
> + QSLIST_ENTRY(InputVisitorAlias) next;
> +} InputVisitorAlias;
> +
> typedef struct StackObject {
> const char *name; /* Name of @obj in its parent, if any */
> QObject *obj; /* QDict or QList being visited */
> @@ -38,6 +61,9 @@ typedef struct StackObject {
> const QListEntry *entry; /* If @obj is QList: unvisited tail */
> unsigned index; /* If @obj is QList: list index of @entry */
>
> + QSLIST_HEAD(, InputVisitorAlias) aliases;
> + int alias_scope_nesting; /* Increase on scope start, decrease on end */
I get what the comment means, but imperative mood is odd for a variable.
"Number of open scopes", perhaps?
> +
> QSLIST_ENTRY(StackObject) node; /* parent */
> } StackObject;
>
I'm afraid I'm too confused about the interface (previous patch) and the
data structures to continue review with reasonable efficiency. I don't
mean to imply either is bad! I'm just confused, that's all.
[...]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor
2020-11-12 17:28 ` [PATCH 3/6] qapi: Simplify full_name_nth() " Kevin Wolf
@ 2021-01-27 13:56 ` Markus Armbruster
2021-01-27 21:42 ` Kevin Wolf
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2021-01-27 13:56 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Instead of counting how many elements from the top of the stack we need
> to ignore until we find the thing we're interested in, we can just
> directly pass the StackObject pointer because all callers already know
> it.
>
> We only need a different way now to tell if we want to know the name of
> something contained in the given StackObject or of the StackObject
> itself. Passing name = NULL is the obvious way to request the latter.
>
> This simplifies the interface and makes it easier to use in cases where
> we have the StackObject, but don't know how many steps down the stack it
> is.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/qobject-input-visitor.c | 38 ++++++++++++++++++------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index a00ac32682..1415561828 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -87,20 +87,16 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
> }
>
> /*
> - * Find the full name of something @qiv is currently visiting.
> - * @qiv is visiting something named @name in the stack of containers
> - * @qiv->stack.
> - * If @n is zero, return its full name.
> - * If @n is positive, return the full name of the @n-th container
> - * counting from the top. The stack of containers must have at least
> - * @n elements.
> - * The returned string is valid until the next full_name_nth(@v) or
> - * destruction of @v.
> + * Find the full name of something named @name in @so which @qiv is
> + * currently visiting. If @name is NULL, find the full name of @so
> + * itself.
> + *
> + * The returned string is valid until the next full_name_so(@qiv) or
> + * destruction of @qiv.
How can this distinguish between a list and its member?
Before the patch:
* list member: n = 0, name = NULL
* list: n = 1, name = NULL
Afterwards?
Checking... yes, regression. Test case:
{"execute": "blockdev-add", "arguments": {"driver": "file", "node-name": "blk0", "filename": "tmp.img"}}
{"return": {}}
{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "node-name": "blk1", "image": "blk0", "take-child-perms": [0]}}
{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms', expected: string"}}
The second command's reply changes from
{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms[0]', expected: string"}}
to
{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms', expected: string"}}
The idea of using @so instead of @n may be salvagable.
[...]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/6] qapi: Add interfaces for alias support to Visitor
2021-01-27 12:51 ` Markus Armbruster
@ 2021-01-27 20:31 ` Kevin Wolf
2021-02-02 13:51 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2021-01-27 20:31 UTC (permalink / raw)
To: Markus Armbruster; +Cc: jsnow, qemu-devel
Am 27.01.2021 um 13:51 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > This adds functions to the Visitor interface that can be used to define
> > aliases and alias scopes.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > include/qapi/visitor-impl.h | 12 ++++++++++++
> > include/qapi/visitor.h | 37 +++++++++++++++++++++++++++++++++++++
> > qapi/qapi-visit-core.c | 21 +++++++++++++++++++++
> > 3 files changed, 70 insertions(+)
> >
> > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> > index 7362c043be..e30da2599c 100644
> > --- a/include/qapi/visitor-impl.h
> > +++ b/include/qapi/visitor-impl.h
> > @@ -113,6 +113,18 @@ struct Visitor
> > The core takes care of the return type in the public interface. */
> > void (*optional)(Visitor *v, const char *name, bool *present);
> >
> > + /*
> > + * Optional; intended for input visitors. If not given, aliases are
> > + * ignored.
> > + */
> > + void (*define_alias)(Visitor *v, const char *alias, const char **source);
> > +
> > + /* Must be set if define_alias is set */
> > + void (*start_alias_scope)(Visitor *v);
> > +
> > + /* Must be set if define_alias is set */
> > + void (*end_alias_scope)(Visitor *v);
> > +
> > /* Must be set */
> > VisitorType type;
> >
> > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> > index ebc19ede7f..9bdc0ee03d 100644
> > --- a/include/qapi/visitor.h
> > +++ b/include/qapi/visitor.h
> > @@ -459,6 +459,43 @@ void visit_end_alternate(Visitor *v, void **obj);
> > */
> > bool visit_optional(Visitor *v, const char *name, bool *present);
> >
> > +/*
> > + * Defines a new alias rule.
> > + *
> > + * If @alias is non-NULL, the member named @alias in the external
> > + * representation of the current struct is defined as an alias for the
>
> Terminology: the big comment uses "object". See also the FIXME in
> visit_start_struct()'s contract.
Ok. Maybe the FIXME should be resolved to avoid this kind of problem.
> > + * member described by @source.
> > + *
> > + * If @alias is NULL, all members of the struct described by @source are
> > + * considered to have alias members with the same key in the current
> > + * struct.
>
> Define "the current struct". I believe it's the object being visited.
Yes.
> What happens if we're currently visiting something other than an object,
> i.e. the root of a tree, or a list?
Then you (= the generated code) shouldn't call the function. Aliases
only make sense for objects (because everything else doesn't have keys).
If you call it anyway, it depends on how you visit the elements of the
list. Currently, I think they are always visited with a NULL name. In
this case the alias should just never apply, but it looks like
propagate_aliases() might actually crash because it doesn't check for
NULL names.
We don't have such callers and I don't think we want to have them, so
I'm not sure if we want to fix anything, and if we do, if the fix should
be tolerating and effectively ignoring such alias definitions or if we
should explicitly assert that the name is non-NULL.
> > + *
> > + * @source is a NULL-terminated array of names that describe the path to
> > + * a member, starting from the currently visited struct.
>
> I'm afraid "describe the path to a member" is too vague. How?
>
> I figure this is what you have in mind:
>
> cur = the currently visited object
> for s in source:
> cur = the member of cur denoted by s
>
> When @cur is indeed an object, then "the member denoted by @s" makes
> sense: you must pass a name when visiting object members, and whatever
> is visited with name @s is the member denoted by @s.
>
> "Must pass a name" is documented in the big comment:
>
> * The @name parameter of visit_type_FOO() describes the relation
> * between this QAPI value and its parent container. When visiting
> * the root of a tree, @name is ignored; when visiting a member of an
> * object, @name is the key associated with the value; when visiting a
> * member of a list, @name is NULL; and when visiting the member of an
> * alternate, @name should equal the name used for visiting the
> * alternate.
>
> But what if @cur is a list? I guess that makes no sense. Say so
> explicitly, please.
Yes, everything but the last element in the path must be an object.
Kevin
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor
2021-01-27 13:06 ` Markus Armbruster
@ 2021-01-27 20:59 ` Kevin Wolf
2021-02-09 12:55 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2021-01-27 20:59 UTC (permalink / raw)
To: Markus Armbruster; +Cc: jsnow, qemu-devel
Am 27.01.2021 um 14:06 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > This makes qobject-input-visitor remember the currently valid aliases in
> > each StackObject. It doesn't actually allow using the aliases yet.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 115 insertions(+)
> >
> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> > index 23843b242e..a00ac32682 100644
> > --- a/qapi/qobject-input-visitor.c
> > +++ b/qapi/qobject-input-visitor.c
> > @@ -29,6 +29,29 @@
> > #include "qemu/cutils.h"
> > #include "qemu/option.h"
> >
> > +typedef struct InputVisitorAlias {
> > + /* Alias name. NULL for any (wildcard alias). */
> > + const char *alias;
> > +
> > + /*
> > + * NULL terminated array representing a path.
> > + * Must contain at least one non-NULL element if alias is not NULL.
>
> What does .alias = NULL, .src[] empty mean?
>
> The previous patch's contract for visit_define_alias():
>
> /*
> * Defines a new alias rule.
> *
> * If @alias is non-NULL, the member named @alias in the external
> * representation of the current struct is defined as an alias for the
> * member described by @source.
> *
> * If @alias is NULL, all members of the struct described by @source are
> * considered to have alias members with the same key in the current
> * struct.
> *
> * @source is a NULL-terminated array of names that describe the path to
> * a member, starting from the currently visited struct.
> *
> * The alias stays valid until the current alias scope ends.
> * visit_start/end_struct() implicitly start/end an alias scope.
> * Additionally, visit_start/end_alias_scope() can be used to explicitly
> * create a nested alias scope.
> */
>
> If I read this correctly, then empty .src[] denotes "the current
> struct", and therefore .alias = NULL, .src[] empty means "all members of
> the current struct are considered to have alias members with the same
> key in the current struct". Which is be a complicated way to accomplish
> nothing.
>
> Am I confused?
Indeed, this doesn't make sense when @alias_so is the currently
processed StackObject. I guess qobject_input_define_alias() should just
forbid this case.
But see below for the relevant case.
> > + */
> > + const char **src;
> > +
> > + /* StackObject in which the alias should be looked for */
> > + struct StackObject *alias_so;
>
> Clear as mud. Is it "the current struct"? If not, what else? Perhaps
> an example would help me understand.
It is the object where the alias was defined.
.alias = NULL, .src[] empty happens after propagating the alias to a
child object, i.e. alias_so is different from the current StackObject.
Let's take a simple SocketAddressLegacy object as an example. Without
aliases, it might look like this:
{ 'type': 'inet',
'data': { 'host': 'localhost', 'port': 1234 } }
We want to eliminate 'data' from the external representation with an
alias, so we map everything in 'data' to the top level. So the actual
external representation that we want to parse in the example is this:
{ 'type': 'inet', 'host': 'localhost', 'port': 1234 }
For the implementation this alias means that while visiting the top
level SocketAddressLegacy object (with StackObject A) we define an
InputVisitorAlias like this:
{
.alias = NULL,
.src = ['data', NULL],
.alias_so = A,
}
When we proceed to visit the 'data' member, we call start_struct, which
creates a new StackObject B. The alias is propagated, stripping 'data'
from the start of .src:
{
.alias = NULL,
.src = [NULL],
.alias_so = A, /* This still refers to A, not B! */
}
Next we're parsing the members of InetSocketAddress (the type of
'data'). The visitor wants to visit 'host' now, but it's not present in
the QDict to parse (StackObject.obj, which is actually an empty QDict in
this case because 'data' wasn't given at all).
So what happens is that it looks at aliases that could provide a value
for this key. Its alias list contains the alias=NULL,src=[NULL] item,
which makes it check if 'host' exists in .alias_so (which is the
StackObject that belongs to the top level SocketAddressLegacy) instead,
and indeed we have a 'host' key there, so we can take the value from
there.
Does this make the mechanism a bit clearer?
> > +
> > + /*
> > + * The alias remains valid as long as the containing StackObject has
>
> What's "the containing StackObject"? I guess it's the one that has this
> InputVisitorAlias in .aliases.
Yes.
> > + * StackObject.alias_scope_nesting >= InputVisitorAlias.scope_nesting
> > + * or until the whole StackObject is removed.
> > + */
> > + int scope_nesting;
> > +
> > + QSLIST_ENTRY(InputVisitorAlias) next;
> > +} InputVisitorAlias;
> > +
> > typedef struct StackObject {
> > const char *name; /* Name of @obj in its parent, if any */
> > QObject *obj; /* QDict or QList being visited */
> > @@ -38,6 +61,9 @@ typedef struct StackObject {
> > const QListEntry *entry; /* If @obj is QList: unvisited tail */
> > unsigned index; /* If @obj is QList: list index of @entry */
> >
> > + QSLIST_HEAD(, InputVisitorAlias) aliases;
> > + int alias_scope_nesting; /* Increase on scope start, decrease on end */
>
> I get what the comment means, but imperative mood is odd for a variable.
> "Number of open scopes", perhaps?
Works for me.
> > +
> > QSLIST_ENTRY(StackObject) node; /* parent */
> > } StackObject;
> >
>
> I'm afraid I'm too confused about the interface (previous patch) and the
> data structures to continue review with reasonable efficiency. I don't
> mean to imply either is bad! I'm just confused, that's all.
I hope the above helps to resolve the confusion. Feel free to come back
with more questions or ping me on IRC if necessary.
Kevin
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor
2021-01-27 13:56 ` Markus Armbruster
@ 2021-01-27 21:42 ` Kevin Wolf
2021-01-28 7:43 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2021-01-27 21:42 UTC (permalink / raw)
To: Markus Armbruster; +Cc: jsnow, qemu-devel
Am 27.01.2021 um 14:56 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Instead of counting how many elements from the top of the stack we need
> > to ignore until we find the thing we're interested in, we can just
> > directly pass the StackObject pointer because all callers already know
> > it.
> >
> > We only need a different way now to tell if we want to know the name of
> > something contained in the given StackObject or of the StackObject
> > itself. Passing name = NULL is the obvious way to request the latter.
> >
> > This simplifies the interface and makes it easier to use in cases where
> > we have the StackObject, but don't know how many steps down the stack it
> > is.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > qapi/qobject-input-visitor.c | 38 ++++++++++++++++++------------------
> > 1 file changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> > index a00ac32682..1415561828 100644
> > --- a/qapi/qobject-input-visitor.c
> > +++ b/qapi/qobject-input-visitor.c
> > @@ -87,20 +87,16 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
> > }
> >
> > /*
> > - * Find the full name of something @qiv is currently visiting.
> > - * @qiv is visiting something named @name in the stack of containers
> > - * @qiv->stack.
> > - * If @n is zero, return its full name.
> > - * If @n is positive, return the full name of the @n-th container
> > - * counting from the top. The stack of containers must have at least
> > - * @n elements.
> > - * The returned string is valid until the next full_name_nth(@v) or
> > - * destruction of @v.
> > + * Find the full name of something named @name in @so which @qiv is
> > + * currently visiting. If @name is NULL, find the full name of @so
> > + * itself.
> > + *
> > + * The returned string is valid until the next full_name_so(@qiv) or
> > + * destruction of @qiv.
>
> How can this distinguish between a list and its member?
>
> Before the patch:
>
> * list member: n = 0, name = NULL
> * list: n = 1, name = NULL
Oh. These two lines were more helpful than the whole function comment
before this patch (which doesn't talk about name = NULL at all).
> Afterwards?
>
> Checking... yes, regression. Test case:
>
> {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name": "blk0", "filename": "tmp.img"}}
> {"return": {}}
> {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "node-name": "blk1", "image": "blk0", "take-child-perms": [0]}}
> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms', expected: string"}}
>
> The second command's reply changes from
>
> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms[0]', expected: string"}}
>
> to
>
> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms', expected: string"}}
>
> The idea of using @so instead of @n may be salvagable.
I can always add a bool parameter that tells (independently from @name)
whether we want the name of a member or of the container.
Though do we really need the name of the container anywhere? The n = 1
case exists in qobject_input_check_list(), but is this a case that can
fail? The pattern how lists are intended to be visited seems to be
calling visit_next_list() until it returns NULL.
The only place where this pattern isn't followed and visit_next_list()
is called outside such a loop, so that we can actually run into the
error in qobject_input_check_list(), is a test case specifically for
this error path.
So should we just declare not visiting all list elements a programming
error and assert instead of constructing an error message that users
will never see?
Kevin
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor
2021-01-27 21:42 ` Kevin Wolf
@ 2021-01-28 7:43 ` Markus Armbruster
2021-01-28 10:57 ` Kevin Wolf
0 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2021-01-28 7:43 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 27.01.2021 um 14:56 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Instead of counting how many elements from the top of the stack we need
>> > to ignore until we find the thing we're interested in, we can just
>> > directly pass the StackObject pointer because all callers already know
>> > it.
>> >
>> > We only need a different way now to tell if we want to know the name of
>> > something contained in the given StackObject or of the StackObject
>> > itself. Passing name = NULL is the obvious way to request the latter.
>> >
>> > This simplifies the interface and makes it easier to use in cases where
>> > we have the StackObject, but don't know how many steps down the stack it
>> > is.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> > qapi/qobject-input-visitor.c | 38 ++++++++++++++++++------------------
>> > 1 file changed, 19 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> > index a00ac32682..1415561828 100644
>> > --- a/qapi/qobject-input-visitor.c
>> > +++ b/qapi/qobject-input-visitor.c
>> > @@ -87,20 +87,16 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
>> > }
>> >
>> > /*
>> > - * Find the full name of something @qiv is currently visiting.
>> > - * @qiv is visiting something named @name in the stack of containers
>> > - * @qiv->stack.
>> > - * If @n is zero, return its full name.
>> > - * If @n is positive, return the full name of the @n-th container
>> > - * counting from the top. The stack of containers must have at least
>> > - * @n elements.
>> > - * The returned string is valid until the next full_name_nth(@v) or
>> > - * destruction of @v.
>> > + * Find the full name of something named @name in @so which @qiv is
>> > + * currently visiting. If @name is NULL, find the full name of @so
>> > + * itself.
>> > + *
>> > + * The returned string is valid until the next full_name_so(@qiv) or
>> > + * destruction of @qiv.
>>
>> How can this distinguish between a list and its member?
>>
>> Before the patch:
>>
>> * list member: n = 0, name = NULL
>> * list: n = 1, name = NULL
>
> Oh. These two lines were more helpful than the whole function comment
> before this patch (which doesn't talk about name = NULL at all).
See, I can write impenetrable comments with the best of them!
The spot that talks about @name is in visitor.h:
* The @name parameter of visit_type_FOO() describes the relation
* between this QAPI value and its parent container. When visiting
* the root of a tree, @name is ignored; when visiting a member of an
* object, @name is the key associated with the value; when visiting a
* member of a list, @name is NULL; and when visiting the member of an
* alternate, @name should equal the name used for visiting the
* alternate.
Many contracts in the same file refer back to it like this:
* @name expresses the relationship of this object to its parent
* container; see the general description of @name above.
The contract here doesn't.
>> Afterwards?
>>
>> Checking... yes, regression. Test case:
>>
>> {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name": "blk0", "filename": "tmp.img"}}
>> {"return": {}}
>> {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "node-name": "blk1", "image": "blk0", "take-child-perms": [0]}}
>> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms', expected: string"}}
>>
>> The second command's reply changes from
>>
>> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms[0]', expected: string"}}
>>
>> to
>>
>> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms', expected: string"}}
>>
>> The idea of using @so instead of @n may be salvagable.
>
> I can always add a bool parameter that tells (independently from @name)
> whether we want the name of a member or of the container.
>
> Though do we really need the name of the container anywhere? The n = 1
> case exists in qobject_input_check_list(), but is this a case that can
> fail? The pattern how lists are intended to be visited seems to be
> calling visit_next_list() until it returns NULL.
Yes, the generated visitors always exhaust the list. But virtual walks
needn't. There's one in hw/ppc/spapr_drc.c:
case FDT_PROP: {
int i;
prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
if (!visit_start_list(v, name, NULL, 0, errp)) {
return;
}
for (i = 0; i < prop_len; i++) {
if (!visit_type_uint8(v, NULL, (uint8_t *)&prop->data[i],
errp)) {
return;
}
}
ok = visit_check_list(v, errp);
visit_end_list(v, NULL);
if (!ok) {
return;
}
break;
}
This visits @prop_len list elements.
If there are fewer, visit_type_uint8() should fail. With the
QObjectInputVisitor, qobject_input_try_get_object() returns null to
qobject_input_get_object(), which then fails.
If there are more, visit_check_list() should fail. With the
QObjectInputVisitor, it's the failure you challenged.
Now, this virtual walk is in a QOM getter, which should only ever be
with an output visitor. Can't fail. Only input visitors can.
> The only place where this pattern isn't followed and visit_next_list()
> is called outside such a loop, so that we can actually run into the
> error in qobject_input_check_list(), is a test case specifically for
> this error path.
See above.
> So should we just declare not visiting all list elements a programming
> error and assert instead of constructing an error message that users
> will never see?
We'd have to restrict virtual walks: if input visitor, then must exhaust
list input. Ugh :)
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor
2021-01-28 7:43 ` Markus Armbruster
@ 2021-01-28 10:57 ` Kevin Wolf
0 siblings, 0 replies; 32+ messages in thread
From: Kevin Wolf @ 2021-01-28 10:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: jsnow, qemu-devel
Am 28.01.2021 um 08:43 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 27.01.2021 um 14:56 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>
> >> > Instead of counting how many elements from the top of the stack we need
> >> > to ignore until we find the thing we're interested in, we can just
> >> > directly pass the StackObject pointer because all callers already know
> >> > it.
> >> >
> >> > We only need a different way now to tell if we want to know the name of
> >> > something contained in the given StackObject or of the StackObject
> >> > itself. Passing name = NULL is the obvious way to request the latter.
> >> >
> >> > This simplifies the interface and makes it easier to use in cases where
> >> > we have the StackObject, but don't know how many steps down the stack it
> >> > is.
> >> >
> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> > ---
> >> > qapi/qobject-input-visitor.c | 38 ++++++++++++++++++------------------
> >> > 1 file changed, 19 insertions(+), 19 deletions(-)
> >> >
> >> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> >> > index a00ac32682..1415561828 100644
> >> > --- a/qapi/qobject-input-visitor.c
> >> > +++ b/qapi/qobject-input-visitor.c
> >> > @@ -87,20 +87,16 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
> >> > }
> >> >
> >> > /*
> >> > - * Find the full name of something @qiv is currently visiting.
> >> > - * @qiv is visiting something named @name in the stack of containers
> >> > - * @qiv->stack.
> >> > - * If @n is zero, return its full name.
> >> > - * If @n is positive, return the full name of the @n-th container
> >> > - * counting from the top. The stack of containers must have at least
> >> > - * @n elements.
> >> > - * The returned string is valid until the next full_name_nth(@v) or
> >> > - * destruction of @v.
> >> > + * Find the full name of something named @name in @so which @qiv is
> >> > + * currently visiting. If @name is NULL, find the full name of @so
> >> > + * itself.
> >> > + *
> >> > + * The returned string is valid until the next full_name_so(@qiv) or
> >> > + * destruction of @qiv.
> >>
> >> How can this distinguish between a list and its member?
> >>
> >> Before the patch:
> >>
> >> * list member: n = 0, name = NULL
> >> * list: n = 1, name = NULL
> >
> > Oh. These two lines were more helpful than the whole function comment
> > before this patch (which doesn't talk about name = NULL at all).
>
> See, I can write impenetrable comments with the best of them!
>
> The spot that talks about @name is in visitor.h:
>
> * The @name parameter of visit_type_FOO() describes the relation
> * between this QAPI value and its parent container. When visiting
> * the root of a tree, @name is ignored; when visiting a member of an
> * object, @name is the key associated with the value; when visiting a
> * member of a list, @name is NULL; and when visiting the member of an
> * alternate, @name should equal the name used for visiting the
> * alternate.
>
> Many contracts in the same file refer back to it like this:
>
> * @name expresses the relationship of this object to its parent
> * container; see the general description of @name above.
>
> The contract here doesn't.
>
> >> Afterwards?
> >>
> >> Checking... yes, regression. Test case:
> >>
> >> {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name": "blk0", "filename": "tmp.img"}}
> >> {"return": {}}
> >> {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "node-name": "blk1", "image": "blk0", "take-child-perms": [0]}}
> >> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms', expected: string"}}
> >>
> >> The second command's reply changes from
> >>
> >> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms[0]', expected: string"}}
> >>
> >> to
> >>
> >> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms', expected: string"}}
> >>
> >> The idea of using @so instead of @n may be salvagable.
> >
> > I can always add a bool parameter that tells (independently from @name)
> > whether we want the name of a member or of the container.
> >
> > Though do we really need the name of the container anywhere? The n = 1
> > case exists in qobject_input_check_list(), but is this a case that can
> > fail? The pattern how lists are intended to be visited seems to be
> > calling visit_next_list() until it returns NULL.
>
> Yes, the generated visitors always exhaust the list. But virtual walks
> needn't.
Ah. Okay, I'll add the bool parameter then.
Kevin
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/6] qapi: Add interfaces for alias support to Visitor
2021-01-27 20:31 ` Kevin Wolf
@ 2021-02-02 13:51 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2021-02-02 13:51 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 27.01.2021 um 13:51 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > This adds functions to the Visitor interface that can be used to define
>> > aliases and alias scopes.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> > include/qapi/visitor-impl.h | 12 ++++++++++++
>> > include/qapi/visitor.h | 37 +++++++++++++++++++++++++++++++++++++
>> > qapi/qapi-visit-core.c | 21 +++++++++++++++++++++
>> > 3 files changed, 70 insertions(+)
>> >
>> > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
>> > index 7362c043be..e30da2599c 100644
>> > --- a/include/qapi/visitor-impl.h
>> > +++ b/include/qapi/visitor-impl.h
>> > @@ -113,6 +113,18 @@ struct Visitor
>> > The core takes care of the return type in the public interface. */
>> > void (*optional)(Visitor *v, const char *name, bool *present);
>> >
>> > + /*
>> > + * Optional; intended for input visitors. If not given, aliases are
>> > + * ignored.
>> > + */
>> > + void (*define_alias)(Visitor *v, const char *alias, const char **source);
>> > +
>> > + /* Must be set if define_alias is set */
>> > + void (*start_alias_scope)(Visitor *v);
>> > +
>> > + /* Must be set if define_alias is set */
>> > + void (*end_alias_scope)(Visitor *v);
>> > +
>> > /* Must be set */
>> > VisitorType type;
>> >
>> > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
>> > index ebc19ede7f..9bdc0ee03d 100644
>> > --- a/include/qapi/visitor.h
>> > +++ b/include/qapi/visitor.h
>> > @@ -459,6 +459,43 @@ void visit_end_alternate(Visitor *v, void **obj);
>> > */
>> > bool visit_optional(Visitor *v, const char *name, bool *present);
>> >
>> > +/*
>> > + * Defines a new alias rule.
>> > + *
>> > + * If @alias is non-NULL, the member named @alias in the external
>> > + * representation of the current struct is defined as an alias for the
>>
>> Terminology: the big comment uses "object". See also the FIXME in
>> visit_start_struct()'s contract.
>
> Ok. Maybe the FIXME should be resolved to avoid this kind of problem.
True. Checking... the churn outside tests/ looks quite tolerable. Feel
free to stick a suitable patch into v2.
>> > + * member described by @source.
>> > + *
>> > + * If @alias is NULL, all members of the struct described by @source are
>> > + * considered to have alias members with the same key in the current
>> > + * struct.
>>
>> Define "the current struct". I believe it's the object being visited.
>
> Yes.
>
>> What happens if we're currently visiting something other than an object,
>> i.e. the root of a tree, or a list?
>
> Then you (= the generated code) shouldn't call the function. Aliases
> only make sense for objects (because everything else doesn't have keys).
>
> If you call it anyway, it depends on how you visit the elements of the
> list. Currently, I think they are always visited with a NULL name. In
> this case the alias should just never apply, but it looks like
> propagate_aliases() might actually crash because it doesn't check for
> NULL names.
>
> We don't have such callers and I don't think we want to have them, so
> I'm not sure if we want to fix anything, and if we do, if the fix should
> be tolerating and effectively ignoring such alias definitions or if we
> should explicitly assert that the name is non-NULL.
I'm like putting "no nonsense" into the contract, then enforce it with
an assertion if practical.
>> > + *
>> > + * @source is a NULL-terminated array of names that describe the path to
>> > + * a member, starting from the currently visited struct.
>>
>> I'm afraid "describe the path to a member" is too vague. How?
>>
>> I figure this is what you have in mind:
>>
>> cur = the currently visited object
>> for s in source:
>> cur = the member of cur denoted by s
>>
>> When @cur is indeed an object, then "the member denoted by @s" makes
>> sense: you must pass a name when visiting object members, and whatever
>> is visited with name @s is the member denoted by @s.
>>
>> "Must pass a name" is documented in the big comment:
>>
>> * The @name parameter of visit_type_FOO() describes the relation
>> * between this QAPI value and its parent container. When visiting
>> * the root of a tree, @name is ignored; when visiting a member of an
>> * object, @name is the key associated with the value; when visiting a
>> * member of a list, @name is NULL; and when visiting the member of an
>> * alternate, @name should equal the name used for visiting the
>> * alternate.
>>
>> But what if @cur is a list? I guess that makes no sense. Say so
>> explicitly, please.
>
> Yes, everything but the last element in the path must be an object.
Got it, thanks.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor
2021-01-27 20:59 ` Kevin Wolf
@ 2021-02-09 12:55 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2021-02-09 12:55 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 27.01.2021 um 14:06 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > This makes qobject-input-visitor remember the currently valid aliases in
>> > each StackObject. It doesn't actually allow using the aliases yet.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> > qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++++++++++++
>> > 1 file changed, 115 insertions(+)
>> >
>> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
>> > index 23843b242e..a00ac32682 100644
>> > --- a/qapi/qobject-input-visitor.c
>> > +++ b/qapi/qobject-input-visitor.c
>> > @@ -29,6 +29,29 @@
>> > #include "qemu/cutils.h"
>> > #include "qemu/option.h"
>> >
>> > +typedef struct InputVisitorAlias {
>> > + /* Alias name. NULL for any (wildcard alias). */
>> > + const char *alias;
>> > +
>> > + /*
>> > + * NULL terminated array representing a path.
>> > + * Must contain at least one non-NULL element if alias is not NULL.
>>
>> What does .alias = NULL, .src[] empty mean?
>>
>> The previous patch's contract for visit_define_alias():
>>
>> /*
>> * Defines a new alias rule.
>> *
>> * If @alias is non-NULL, the member named @alias in the external
>> * representation of the current struct is defined as an alias for the
>> * member described by @source.
>> *
>> * If @alias is NULL, all members of the struct described by @source are
>> * considered to have alias members with the same key in the current
>> * struct.
>> *
>> * @source is a NULL-terminated array of names that describe the path to
>> * a member, starting from the currently visited struct.
>> *
>> * The alias stays valid until the current alias scope ends.
>> * visit_start/end_struct() implicitly start/end an alias scope.
>> * Additionally, visit_start/end_alias_scope() can be used to explicitly
>> * create a nested alias scope.
>> */
>>
>> If I read this correctly, then empty .src[] denotes "the current
>> struct", and therefore .alias = NULL, .src[] empty means "all members of
>> the current struct are considered to have alias members with the same
>> key in the current struct". Which is be a complicated way to accomplish
>> nothing.
>>
>> Am I confused?
>
> Indeed, this doesn't make sense when @alias_so is the currently
> processed StackObject. I guess qobject_input_define_alias() should just
> forbid this case.
Yes, please.
> But see below for the relevant case.
>
>> > + */
>> > + const char **src;
>> > +
>> > + /* StackObject in which the alias should be looked for */
>> > + struct StackObject *alias_so;
>>
>> Clear as mud. Is it "the current struct"? If not, what else? Perhaps
>> an example would help me understand.
>
> It is the object where the alias was defined.
>
> .alias = NULL, .src[] empty happens after propagating the alias to a
> child object, i.e. alias_so is different from the current StackObject.
>
> Let's take a simple SocketAddressLegacy object as an example. Without
> aliases, it might look like this:
>
> { 'type': 'inet',
> 'data': { 'host': 'localhost', 'port': 1234 } }
>
> We want to eliminate 'data' from the external representation with an
> alias, so we map everything in 'data' to the top level. So the actual
> external representation that we want to parse in the example is this:
>
> { 'type': 'inet', 'host': 'localhost', 'port': 1234 }
>
> For the implementation this alias means that while visiting the top
> level SocketAddressLegacy object (with StackObject A) we define an
> InputVisitorAlias like this:
>
> {
> .alias = NULL,
Make all members of ...
> .src = ['data', NULL],
... the object .data also available in ...
> .alias_so = A,
the object corresponding to A, which is the object we're visiting right
now (A is on top of the stack).
Correct?
> }
>
> When we proceed to visit the 'data' member, we call start_struct, which
> creates a new StackObject B. The alias is propagated, stripping 'data'
> from the start of .src:
>
> {
> .alias = NULL,
Make all members of ...
> .src = [NULL],
... the current object also available in ...
> .alias_so = A, /* This still refers to A, not B! */
the object corresponding to A, which is the object containing the one
we're visiting right now (A right below the top of the stack).
Correct?
> }
>
> Next we're parsing the members of InetSocketAddress (the type of
> 'data'). The visitor wants to visit 'host' now, but it's not present in
> the QDict to parse (StackObject.obj, which is actually an empty QDict in
> this case because 'data' wasn't given at all).
Wait a sec! Doesn't qobject_input_start_struct(v, "data", ...) *fail*
when "data" is absent? Oh, looks like you're messing with that in PATCH
4. Forward reference only in your explanation, not in this patch, which
"doesn't actually allow using the aliases yet".
> So what happens is that it looks at aliases that could provide a value
> for this key. Its alias list contains the alias=NULL,src=[NULL] item,
> which makes it check if 'host' exists in .alias_so (which is the
> StackObject that belongs to the top level SocketAddressLegacy) instead,
> and indeed we have a 'host' key there, so we can take the value from
> there.
When src[0] isn't null, this InputVisitorAlias is to be applied deeper
in the visit, and we ignore it here.
Correct?
> Does this make the mechanism a bit clearer?
Feels like it :)
>> > +
>> > + /*
>> > + * The alias remains valid as long as the containing StackObject has
>>
>> What's "the containing StackObject"? I guess it's the one that has this
>> InputVisitorAlias in .aliases.
>
> Yes.
>
>> > + * StackObject.alias_scope_nesting >= InputVisitorAlias.scope_nesting
>> > + * or until the whole StackObject is removed.
>> > + */
>> > + int scope_nesting;
>> > +
>> > + QSLIST_ENTRY(InputVisitorAlias) next;
>> > +} InputVisitorAlias;
>> > +
>> > typedef struct StackObject {
>> > const char *name; /* Name of @obj in its parent, if any */
>> > QObject *obj; /* QDict or QList being visited */
>> > @@ -38,6 +61,9 @@ typedef struct StackObject {
>> > const QListEntry *entry; /* If @obj is QList: unvisited tail */
>> > unsigned index; /* If @obj is QList: list index of @entry */
>> >
>> > + QSLIST_HEAD(, InputVisitorAlias) aliases;
We need a list because any number of aliases may apply here (src[0]
null) or deeper in the stack (not null).
Correct?
>> > + int alias_scope_nesting; /* Increase on scope start, decrease on end */
>>
>> I get what the comment means, but imperative mood is odd for a variable.
>> "Number of open scopes", perhaps?
>
> Works for me.
>
>> > +
>> > QSLIST_ENTRY(StackObject) node; /* parent */
>> > } StackObject;
>> >
>>
>> I'm afraid I'm too confused about the interface (previous patch) and the
>> data structures to continue review with reasonable efficiency. I don't
>> mean to imply either is bad! I'm just confused, that's all.
>
> I hope the above helps to resolve the confusion. Feel free to come back
> with more questions or ping me on IRC if necessary.
Thanks!
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor
2020-11-12 17:28 ` [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor Kevin Wolf
2021-01-27 13:06 ` Markus Armbruster
@ 2021-02-09 12:57 ` Markus Armbruster
2021-02-11 16:27 ` Kevin Wolf
1 sibling, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2021-02-09 12:57 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Let me look at the actual code now Kevin reduced my confusion about the
interface and the data structures.
Kevin Wolf <kwolf@redhat.com> writes:
> This makes qobject-input-visitor remember the currently valid aliases in
> each StackObject. It doesn't actually allow using the aliases yet.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++++++++++++
> 1 file changed, 115 insertions(+)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 23843b242e..a00ac32682 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -29,6 +29,29 @@
> #include "qemu/cutils.h"
> #include "qemu/option.h"
>
> +typedef struct InputVisitorAlias {
> + /* Alias name. NULL for any (wildcard alias). */
> + const char *alias;
> +
> + /*
> + * NULL terminated array representing a path.
> + * Must contain at least one non-NULL element if alias is not NULL.
> + */
> + const char **src;
> +
> + /* StackObject in which the alias should be looked for */
> + struct StackObject *alias_so;
> +
> + /*
> + * The alias remains valid as long as the containing StackObject has
> + * StackObject.alias_scope_nesting >= InputVisitorAlias.scope_nesting
> + * or until the whole StackObject is removed.
> + */
> + int scope_nesting;
> +
> + QSLIST_ENTRY(InputVisitorAlias) next;
> +} InputVisitorAlias;
> +
> typedef struct StackObject {
> const char *name; /* Name of @obj in its parent, if any */
> QObject *obj; /* QDict or QList being visited */
> @@ -38,6 +61,9 @@ typedef struct StackObject {
> const QListEntry *entry; /* If @obj is QList: unvisited tail */
> unsigned index; /* If @obj is QList: list index of @entry */
>
> + QSLIST_HEAD(, InputVisitorAlias) aliases;
> + int alias_scope_nesting; /* Increase on scope start, decrease on end */
> +
> QSLIST_ENTRY(StackObject) node; /* parent */
> } StackObject;
>
> @@ -203,6 +229,38 @@ static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv,
> return qstring_get_str(qstr);
> }
>
> +/*
> + * Propagates aliases from the parent StackObject @src to its direct
> + * child StackObject @dst, which is representing the child struct @name.
@name must equal dst->name, I think. Drop the parameter?
> + *
> + * Every alias whose source path begins with @name and which still
> + * applies in @dst (i.e. it is either a wildcard alias or has at least
> + * one more source path element) is propagated to @dst with the first
I'm not sure I get the parenthesis. Perhaps the code will enlighten me.
> + * element (i.e. @name) removed from the source path.
> + */
> +static void propagate_aliases(StackObject *dst, StackObject *src,
> + const char *name)
> +{
> + InputVisitorAlias *a;
> +
> + QSLIST_FOREACH(a, &src->aliases, next) {
> + if (!a->src[0] || strcmp(a->src[0], name)) {
> + continue;
> + }
We only look at the aliases that apply to @dst or below. They do only
when ->src[0] equals dst->name. Makes sense.
> + if (a->src[1] || !a->alias) {
If a->src[1], the alias applies below @dst, not to @dst. To get it to
the place where it applies, we first need to propagate to @dst.
Else, the alias applies to @dst. If !a->alias, we're looking at a
wildcard alias, i.e. all members of the object described by @dst are
aliased. Why do we need to propagate only wildcard aliases to @dst?
> + InputVisitorAlias *alias = g_new(InputVisitorAlias, 1);
> +
> + *alias = (InputVisitorAlias) {
> + .alias = a->alias,
> + .alias_so = a->alias_so,
> + .src = &a->src[1],
> + };
> +
> + QSLIST_INSERT_HEAD(&dst->aliases, alias, next);
> + }
> + }
> +}
> +
> static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
> const char *name,
> QObject *obj, void *qapi)
> @@ -226,6 +284,9 @@ static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
> g_hash_table_insert(h, (void *)qdict_entry_key(entry), NULL);
> }
> tos->h = h;
> + if (!QSLIST_EMPTY(&qiv->stack)) {
> + propagate_aliases(tos, QSLIST_FIRST(&qiv->stack), name);
> + }
What if QSLIST_EMPTY(&qiv->stack) and tos->aliases contains aliases that
apply deeper?
> } else {
> assert(qlist);
> tos->entry = qlist_first(qlist);
> @@ -257,10 +318,17 @@ static bool qobject_input_check_struct(Visitor *v, Error **errp)
>
> static void qobject_input_stack_object_free(StackObject *tos)
> {
> + InputVisitorAlias *a;
> +
> if (tos->h) {
> g_hash_table_unref(tos->h);
> }
>
> + while ((a = QSLIST_FIRST(&tos->aliases))) {
> + QSLIST_REMOVE_HEAD(&tos->aliases, next);
> + g_free(a);
> + }
> +
> g_free(tos);
> }
>
> @@ -274,6 +342,50 @@ static void qobject_input_pop(Visitor *v, void **obj)
> qobject_input_stack_object_free(tos);
> }
>
> +static void qobject_input_start_alias_scope(Visitor *v)
> +{
> + QObjectInputVisitor *qiv = to_qiv(v);
> + StackObject *tos = QSLIST_FIRST(&qiv->stack);
> +
> + tos->alias_scope_nesting++;
> +}
> +
> +static void qobject_input_end_alias_scope(Visitor *v)
> +{
> + QObjectInputVisitor *qiv = to_qiv(v);
> + StackObject *tos = QSLIST_FIRST(&qiv->stack);
> + InputVisitorAlias *a, *next;
> +
> + assert(tos->alias_scope_nesting > 0);
> + tos->alias_scope_nesting--;
> +
> + QSLIST_FOREACH_SAFE(a, &tos->aliases, next, next) {
> + if (a->scope_nesting > tos->alias_scope_nesting) {
> + QSLIST_REMOVE(&tos->aliases, a, InputVisitorAlias, next);
> + g_free(a);
> + }
> + }
> +}
> +
> +static void qobject_input_define_alias(Visitor *v, const char *alias_name,
> + const char **source)
> +{
> + QObjectInputVisitor *qiv = to_qiv(v);
> + StackObject *tos = QSLIST_FIRST(&qiv->stack);
> + InputVisitorAlias *alias = g_new(InputVisitorAlias, 1);
> +
> + /* The source path can only be empty for wildcard aliases */
> + assert(source[0] || !alias_name);
Possibly related: the "What does .alias = NULL, .src[] empty mean?" we
discussed previously.
> +
> + *alias = (InputVisitorAlias) {
> + .alias = alias_name,
> + .alias_so = tos,
> + .src = source,
> + };
> +
> + QSLIST_INSERT_HEAD(&tos->aliases, alias, next);
> +}
> +
> static bool qobject_input_start_struct(Visitor *v, const char *name, void **obj,
> size_t size, Error **errp)
> {
> @@ -696,6 +808,9 @@ static QObjectInputVisitor *qobject_input_visitor_base_new(QObject *obj)
> v->visitor.end_list = qobject_input_end_list;
> v->visitor.start_alternate = qobject_input_start_alternate;
> v->visitor.optional = qobject_input_optional;
> + v->visitor.define_alias = qobject_input_define_alias;
> + v->visitor.start_alias_scope = qobject_input_start_alias_scope;
> + v->visitor.end_alias_scope = qobject_input_end_alias_scope;
> v->visitor.free = qobject_input_free;
>
> v->root = qobject_ref(obj);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/6] qapi: Apply aliases in qobject-input-visitor
2020-11-12 17:28 ` [PATCH 4/6] qapi: Apply aliases " Kevin Wolf
@ 2021-02-09 16:02 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2021-02-09 16:02 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> When looking for an object in a struct in the external representation,
> check not only the currently visited struct, but also whether an alias
> in the current StackObject matches and try to fetch the value from the
> alias then. Providing two values for the same object through different
> aliases is an error.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
I had to read this patch mostly backwards to make sense of it. I
recommend you read my review mostly backwards, too: forward within each
function, backwards otherwise.
I sometimes put helpers after their users to avoid that.
> ---
> qapi/qobject-input-visitor.c | 169 +++++++++++++++++++++++++++++++++--
> 1 file changed, 160 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 1415561828..faca5b6b55 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -74,6 +74,8 @@ struct QObjectInputVisitor {
> QObject *root;
> bool keyval; /* Assume @root made with keyval_parse() */
>
> + QDict *empty_qdict; /* Used for implicit objects */
> +
> /* Stack of objects being visited (all entries will be either
> * QDict or QList). */
> QSLIST_HEAD(, StackObject) stack;
> @@ -141,9 +143,139 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> return full_name_so(qiv, name, tos);
> }
>
> +static bool find_object_member(QObjectInputVisitor *qiv,
> + StackObject **so, const char **name,
> + bool *implicit_object, Error **errp);
> +
> +static bool alias_present(QObjectInputVisitor *qiv,
> + InputVisitorAlias *a, const char *name)
> +{
> + StackObject *so = a->alias_so;
> +
> + /*
> + * The passed source @name is only relevant for wildcard aliases which
> + * don't have a separate name, otherwise we use the alias name.
> + */
> + if (a->alias) {
> + name = a->alias;
> + }
> +
> + if (!find_object_member(qiv, &so, &name, NULL, NULL)) {
> + return false;
> + }
> +
> + /*
> + * Every source can be used only once. If a value in the input would end up
> + * being used twice through aliases, we'll fail the second access.
> + */
> + if (!g_hash_table_contains(so->h, name)) {
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static bool alias_source_matches(QObjectInputVisitor *qiv,
> + StackObject *so, InputVisitorAlias *a,
> + const char *name, bool *implicit_object)
> +{
> + if (a->src[0] == NULL) {
> + assert(a->alias == NULL);
> + return true;
> + }
> +
> + if (!strcmp(a->src[0], name)) {
> + if (a->alias && a->src[1] == NULL) {
> + /*
> + * We're matching an exact member, the source for this alias is
> + * immediately in @so.
> + */
> + return true;
> + } else if (implicit_object) {
> + /*
> + * We're only looking at a prefix of the source path for the alias.
> + * If the input contains no object of the requested name, we will
> + * implicitly create an empty one so that the alias can still be
> + * used.
> + *
> + * We want to create the implicit object only if the alias is
> + * actually used, but we can't tell here for wildcard aliases (only
> + * a later visitor call will determine this). This means that
> + * wildcard aliases must never have optional keys in their source
> + * path.
> + */
> + if (!a->alias || alias_present(qiv, a, a->alias)) {
> + *implicit_object = true;
> + }
> + }
> + }
> +
> + return false;
> +}
> +
> +static bool find_object_member(QObjectInputVisitor *qiv,
> + StackObject **so, const char **name,
> + bool *implicit_object, Error **errp)
> +{
> + StackObject *cur_so = *so;
> + QDict *qdict = qobject_to(QDict, cur_so->obj);
> + const char *found = NULL;
> + bool found_is_wildcard = false;
> + InputVisitorAlias *a;
> +
> + if (implicit_object) {
> + *implicit_object = false;
> + }
> +
> + /* Directly present in the container */
> + if (qdict_haskey(qdict, *name)) {
> + found = *name;
> + }
> +
> + /*
> + * Find aliases whose source path matches @name in this StackObject. We can
> + * then get the value with the key a->alias from a->alias_so.
> + */
> + QSLIST_FOREACH(a, &cur_so->aliases, next) {
> + if (a->alias == NULL && found) {
> + /*
> + * Skip wildcard aliases if we already have a match. This is
> + * not a conflict that should result in an error.
> + */
> + continue;
> + }
> +
> + if (!alias_source_matches(qiv, cur_so, a, *name, implicit_object)) {
> + continue;
> + }
> +
> + if (!alias_present(qiv, a, *name)) {
> + continue;
> + }
> +
> + if (found && !found_is_wildcard) {
> + error_setg(errp, "Value for parameter %s was already given "
> + "through an alias", full_name_so(qiv, *name, *so));
> + return false;
> + } else {
> + found = a->alias ?: *name;
> + *so = a->alias_so;
> + found_is_wildcard = !a->alias;
> + }
> + }
> +
> + /* Chained aliases: *so/found might be the source of another alias */
> + if (found && (*so != cur_so || found != *name)) {
> + find_object_member(qiv, so, &found, NULL, errp);
> + }
> +
> + *name = found;
> + return found;
> +}
> +
> static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
> const char *name,
> - bool consume)
> + bool consume, Error **errp)
> {
> StackObject *tos;
> QObject *qobj;
> @@ -161,10 +293,24 @@ static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
> assert(qobj);
>
> if (qobject_type(qobj) == QTYPE_QDICT) {
> - assert(name);
> - ret = qdict_get(qobject_to(QDict, qobj), name);
> - if (tos->h && consume && ret) {
> - bool removed = g_hash_table_remove(tos->h, name);
> + StackObject *so = tos;
> + const char *key = name;
> + bool implicit_object;
> +
> + assert(key);
> + if (!find_object_member(qiv, &so, &key, &implicit_object, errp)) {
I guess this changes @so, @key exactly when it follows an alias.
We'll see when @implicit_object is set when we get to the spot that sets
it (remember, reading backwards).
> + if (implicit_object) {
> + if (!qiv->empty_qdict) {
> + qiv->empty_qdict = qdict_new();
> + }
> + return QOBJECT(qiv->empty_qdict);
Returns a soft reference (not to be qobject_unref()'ed), which is
correct.
> + } else {
> + return NULL;
> + }
> + }
> + ret = qdict_get(qobject_to(QDict, so->obj), key);
> + if (so->h && consume && ret) {
> + bool removed = g_hash_table_remove(so->h, key);
> assert(removed);
> }
> } else {
Cases:
* !find_object_member() && implicit_object && no error set
Return a shared empty QDict, no error set.
* !find_object_member() && implicit_object && error set
Must not happen.
* !find_object_member() && !implicit_object && no error set
Return null, no error set.
* !find_object_member() && !implicit_object && error set
Return null, error set.
* find_object_member() && no error set
Return input.
implicit_object is ignored.
* find_object_member() && error set
Must not happen.
I can only guess what these cases mean.
find_object_member() is too complicated to go without a written
contract. Please add one.
I'd prefer to see it before I continue review.
> @@ -190,9 +336,10 @@ static QObject *qobject_input_get_object(QObjectInputVisitor *qiv,
> const char *name,
> bool consume, Error **errp)
> {
> - QObject *obj = qobject_input_try_get_object(qiv, name, consume);
> + ERRP_GUARD();
> + QObject *obj = qobject_input_try_get_object(qiv, name, consume, errp);
>
> - if (!obj) {
> + if (!obj && !*errp) {
Likewise (below, not above; we're reading backwards).
> error_setg(errp, QERR_MISSING_PARAMETER, full_name(qiv, name));
> }
> return obj;
> @@ -764,13 +911,16 @@ static bool qobject_input_type_size_keyval(Visitor *v, const char *name,
> static void qobject_input_optional(Visitor *v, const char *name, bool *present)
> {
> QObjectInputVisitor *qiv = to_qiv(v);
> - QObject *qobj = qobject_input_try_get_object(qiv, name, false);
> + Error *local_err = NULL;
> + QObject *qobj = qobject_input_try_get_object(qiv, name, false, &local_err);
>
> - if (!qobj) {
> + /* If there was an error, let the caller try and run into the error */
> + if (!qobj && !local_err) {
> *present = false;
> return;
> }
>
> + error_free(local_err);
> *present = true;
> }
>
Awkward.
Before the patch, qobject_input_try_get_object() returns the input for
@name, or else null.
Afterwards, we have three cases:
* Return non-null, no error set: this is the input for name, as before.
* Return null, no error set: there is no input for name.
* Return null, error set: "Value for parameter %s was already given
through an alias". We'll see what that means when we get to the spot
that sets this error (remember, reading backwards).
I can't yet say whether this could be avoided.
> @@ -785,6 +935,7 @@ static void qobject_input_free(Visitor *v)
> qobject_input_stack_object_free(tos);
> }
>
> + qobject_unref(qiv->empty_qdict);
> qobject_unref(qiv->root);
> if (qiv->errname) {
> g_string_free(qiv->errname, TRUE);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] qapi: Add support for aliases
2020-11-12 17:28 ` [PATCH 5/6] qapi: Add support for aliases Kevin Wolf
2020-11-12 18:34 ` Eric Blake
@ 2021-02-10 9:17 ` Markus Armbruster
2021-02-10 12:26 ` Kevin Wolf
2021-02-10 14:29 ` Markus Armbruster
2 siblings, 1 reply; 32+ messages in thread
From: Markus Armbruster @ 2021-02-10 9:17 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Introduce alias definitions for object types (structs and unions). This
> allows using the same QAPI type and visitor for many syntax variations
> that exist in the external representation, like between QMP and the
> command line. It also provides a new tool for evolving the schema while
> maintaining backwards compatibility during a deprecation period.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> docs/devel/qapi-code-gen.txt | 37 +++++++++++++++++++++++---
> docs/sphinx/qapidoc.py | 2 +-
> scripts/qapi/expr.py | 34 +++++++++++++++++++++--
> scripts/qapi/schema.py | 27 +++++++++++++++----
> scripts/qapi/types.py | 4 ++-
> scripts/qapi/visit.py | 33 ++++++++++++++++++++---
> tests/qapi-schema/test-qapi.py | 7 ++++-
> tests/qapi-schema/double-type.err | 2 +-
> tests/qapi-schema/unknown-expr-key.err | 2 +-
> 9 files changed, 130 insertions(+), 18 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 6906a06ad2..6da14d5275 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -231,7 +231,8 @@ Syntax:
> 'data': MEMBERS,
> '*base': STRING,
> '*if': COND,
> - '*features': FEATURES }
> + '*features': FEATURES,
> + '*aliases': ALIASES }
> MEMBERS = { MEMBER, ... }
> MEMBER = STRING : TYPE-REF
> | STRING : { 'type': TYPE-REF,
Missing: a forward reference, like we have for 'if' and 'features'.
Here's the obvious one:
The optional 'if' member specifies a conditional. See "Configuring
the schema" below for more on this.
The optional 'features' member specifies features. See "Features"
below for more on this.
+The optional 'aliases' member specifies aliases. See "Aliases" below
+for more on this.
> @@ -286,13 +287,15 @@ Syntax:
> UNION = { 'union': STRING,
> 'data': BRANCHES,
> '*if': COND,
> - '*features': FEATURES }
> + '*features': FEATURES,
> + '*aliases': ALIASES }
> | { 'union': STRING,
> 'data': BRANCHES,
> 'base': ( MEMBERS | STRING ),
> 'discriminator': STRING,
> '*if': COND,
> - '*features': FEATURES }
> + '*features': FEATURES,
> + '*aliases': ALIASES }
> BRANCHES = { BRANCH, ... }
> BRANCH = STRING : TYPE-REF
> | STRING : { 'type': TYPE-REF, '*if': COND }
Likewise.
> @@ -837,6 +840,34 @@ shows a conditional entity only when the condition is satisfied in
> this particular build.
>
>
> +=== Aliases ===
> +
> +Object types, including structs and unions, can contain alias
> +definitions.
> +
> +Aliases define alternative member names that may be used in the
> +external representation to provide a value for a member in the same
> +object or in a nested object.
"or one if its sub-objects"? Not sure which is better.
> +
> +Syntax:
> + ALIAS = { '*alias': STRING,
> + 'source': [ STRING, ... ] }
You used non-terminal ALIASES above. Please define it here.
I have doubts about the name 'alias'. The alias is the complete thing,
and 'alias' is just one property of the complete thing. I think 'name'
would be better. Further evidence: below, you write "If 'alias' is
present" and "If 'alias' is not present". I think both read better with
'name' instead of 'alias'.
> +
> +'source' is a list of member names representing the path to an object
> +member, starting from the type where the alias definition is
> +specified.
May 'source' be empty? More on that below.
"where the definition is specified" feels a bit awkward, and "path"
slightly hand-wavy. Let me try induction:
'source' is a list of member names. The first name is resolved in
the same object. Each subsequent member is resolved in the object
named by the preceding member.
Differently awkward, I guess.
> It may refer to another alias name. It is allowed to use
> +a path that doesn't necessarily match an existing member in every
> +variant or even at all; in this case, the alias remains unused.
Aha! Knowing this would've saved me some trouble in reviewing code.
I wrote on PATCH 1:
I think updating the big comment in visitor.h for aliases would help.
Let's postpone it until I've seen the rest of the series.
We can cover unused aliases right there. Whether they also need to go
into contracts we'll see.
What if only a *prefix* of 'source' matches? E.g.
'source': ['eins', 'zwei', 'drei']
and we have an object-valued member 'eins' (match), which has a member
'zwei' (match), which is *not* an object. Is that an error? Is it
caught?
> +
> +If 'alias' is present, then the single member referred to by 'source'
> +is made accessible with the name given in 'alias' in the type where
> +the alias definition is specified.
'source' may not be empty here. Correct?
If yes, please spell it out.
Double-checking I got it... Say we have
'alias': 'foo',
'source': ['bar', 'baz']
where 'bar' is an object with a member 'baz'.
Then input "foo": FOOVAL gets interpreted like "bar": {"baz": FOOVAL}.
If input also contains "bar", we merge. Duplicates are an error.
This is the case even when 'baz' is an object. If you want to alias
member 'foo' of 'baz', you have to say
'alias': 'foo',
'source': ['bar', 'baz', 'foo']
Correct?
> +
> +If 'alias' is not present, then all members in the object referred to
> +by 'source' are made accessible in the type where the alias definition
> +is specified with the same name as they have in 'source'.
'source' may not be empty here, either. Correct?
If yes, please spell it out, and make sure the code catches it.
What if it resolve to a non-object? Is that an error? Is it caught?
Continuing the double-checking... Say we have
# alias missing
'source': ['gnu']
where 'gnu' is an object with a member 'gnat'.
Input "gnat": GNATVAL gets interpreted like "gnu": {"gnat": GNATVAL}.
Correct?
The document could use examples. Feel free to steal mine.
I think we should talk about 'alias' first, and only then about
'source'. It matches their order in the schema, and also matches how I
think about aliases, namely "this name actually means that". Here,
"this name" is 'alias', and "that" is 'source'.
> +
> +
Don't get deceived by my comments; this is a pretty good start.
I wish I had studied this part before PATCH 1.
> === Documentation comments ===
>
> A multi-line comment that starts and ends with a '##' line is a
I intend to look at the remainder shortly.
[...]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] qapi: Add support for aliases
2021-02-10 9:17 ` Markus Armbruster
@ 2021-02-10 12:26 ` Kevin Wolf
2021-02-10 13:47 ` Markus Armbruster
0 siblings, 1 reply; 32+ messages in thread
From: Kevin Wolf @ 2021-02-10 12:26 UTC (permalink / raw)
To: Markus Armbruster; +Cc: jsnow, qemu-devel
Am 10.02.2021 um 10:17 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Introduce alias definitions for object types (structs and unions). This
> > allows using the same QAPI type and visitor for many syntax variations
> > that exist in the external representation, like between QMP and the
> > command line. It also provides a new tool for evolving the schema while
> > maintaining backwards compatibility during a deprecation period.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > docs/devel/qapi-code-gen.txt | 37 +++++++++++++++++++++++---
> > docs/sphinx/qapidoc.py | 2 +-
> > scripts/qapi/expr.py | 34 +++++++++++++++++++++--
> > scripts/qapi/schema.py | 27 +++++++++++++++----
> > scripts/qapi/types.py | 4 ++-
> > scripts/qapi/visit.py | 33 ++++++++++++++++++++---
> > tests/qapi-schema/test-qapi.py | 7 ++++-
> > tests/qapi-schema/double-type.err | 2 +-
> > tests/qapi-schema/unknown-expr-key.err | 2 +-
> > 9 files changed, 130 insertions(+), 18 deletions(-)
> >
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index 6906a06ad2..6da14d5275 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -231,7 +231,8 @@ Syntax:
> > 'data': MEMBERS,
> > '*base': STRING,
> > '*if': COND,
> > - '*features': FEATURES }
> > + '*features': FEATURES,
> > + '*aliases': ALIASES }
> > MEMBERS = { MEMBER, ... }
> > MEMBER = STRING : TYPE-REF
> > | STRING : { 'type': TYPE-REF,
>
> Missing: a forward reference, like we have for 'if' and 'features'.
> Here's the obvious one:
>
> The optional 'if' member specifies a conditional. See "Configuring
> the schema" below for more on this.
>
> The optional 'features' member specifies features. See "Features"
> below for more on this.
>
> +The optional 'aliases' member specifies aliases. See "Aliases" below
> +for more on this.
>
> > @@ -286,13 +287,15 @@ Syntax:
> > UNION = { 'union': STRING,
> > 'data': BRANCHES,
> > '*if': COND,
> > - '*features': FEATURES }
> > + '*features': FEATURES,
> > + '*aliases': ALIASES }
> > | { 'union': STRING,
> > 'data': BRANCHES,
> > 'base': ( MEMBERS | STRING ),
> > 'discriminator': STRING,
> > '*if': COND,
> > - '*features': FEATURES }
> > + '*features': FEATURES,
> > + '*aliases': ALIASES }
> > BRANCHES = { BRANCH, ... }
> > BRANCH = STRING : TYPE-REF
> > | STRING : { 'type': TYPE-REF, '*if': COND }
>
> Likewise.
>
> > @@ -837,6 +840,34 @@ shows a conditional entity only when the condition is satisfied in
> > this particular build.
> >
> >
> > +=== Aliases ===
> > +
> > +Object types, including structs and unions, can contain alias
> > +definitions.
> > +
> > +Aliases define alternative member names that may be used in the
> > +external representation to provide a value for a member in the same
> > +object or in a nested object.
>
> "or one if its sub-objects"? Not sure which is better.
"nested object" feels a little clearer to me, but not that it's a big
difference. If you feel "sub-object" is better, I can use that.
> > +
> > +Syntax:
> > + ALIAS = { '*alias': STRING,
> > + 'source': [ STRING, ... ] }
>
> You used non-terminal ALIASES above. Please define it here.
>
> I have doubts about the name 'alias'. The alias is the complete thing,
> and 'alias' is just one property of the complete thing. I think 'name'
> would be better. Further evidence: below, you write "If 'alias' is
> present" and "If 'alias' is not present". I think both read better with
> 'name' instead of 'alias'.
Works for me.
> > +
> > +'source' is a list of member names representing the path to an object
> > +member, starting from the type where the alias definition is
> > +specified.
>
> May 'source' be empty? More on that below.
No. Empty 'source' isn't the path to any object member, so it doesn't
meet the requirement. If you prefer, we can explicitly specify a
"non-empty list".
> "where the definition is specified" feels a bit awkward, and "path"
> slightly hand-wavy. Let me try induction:
>
> 'source' is a list of member names. The first name is resolved in
> the same object. Each subsequent member is resolved in the object
> named by the preceding member.
>
> Differently awkward, I guess.
Now you've left out what the purpose of it is. I think I'll combine your
version with my first part ("'source' is a list of member names
representing the path to an object member").
> > It may refer to another alias name. It is allowed to use
> > +a path that doesn't necessarily match an existing member in every
> > +variant or even at all; in this case, the alias remains unused.
>
> Aha! Knowing this would've saved me some trouble in reviewing code.
>
> I wrote on PATCH 1:
>
> I think updating the big comment in visitor.h for aliases would help.
> Let's postpone it until I've seen the rest of the series.
>
> We can cover unused aliases right there. Whether they also need to go
> into contracts we'll see.
Ok. I assume updating that big comment is still postponed because you
saw the series, but didn't actually review all of it yet?
> What if only a *prefix* of 'source' matches? E.g.
>
> 'source': ['eins', 'zwei', 'drei']
>
> and we have an object-valued member 'eins' (match), which has a member
> 'zwei' (match), which is *not* an object. Is that an error? Is it
> caught?
This feels like a realistic case to me when 'eins' is a union type where
some variants contain an object 'zwei' with a member 'drei' and others
have 'zwei' as a non-object member.
In this case, we want the alias not to match in the non-object 'zwei'
case, but we do want it to match in another variant. So it is
intentionally not an error.
The QAPI generator could try to prove that there is at least one variant
where the alias would actually be applied, but just leaving it unused
when it doesn't match anywhere seemed good enough to me.
> > +
> > +If 'alias' is present, then the single member referred to by 'source'
> > +is made accessible with the name given in 'alias' in the type where
> > +the alias definition is specified.
>
> 'source' may not be empty here. Correct?
>
> If yes, please spell it out.
Yes. Does spelling it out more explicitly in the description of 'source'
suffice?
> Double-checking I got it... Say we have
>
> 'alias': 'foo',
> 'source': ['bar', 'baz']
>
> where 'bar' is an object with a member 'baz'.
>
> Then input "foo": FOOVAL gets interpreted like "bar": {"baz": FOOVAL}.
>
> If input also contains "bar", we merge. Duplicates are an error.
>
> This is the case even when 'baz' is an object. If you want to alias
> member 'foo' of 'baz', you have to say
>
> 'alias': 'foo',
> 'source': ['bar', 'baz', 'foo']
>
> Correct?
Correct.
> > +
> > +If 'alias' is not present, then all members in the object referred to
> > +by 'source' are made accessible in the type where the alias definition
> > +is specified with the same name as they have in 'source'.
>
> 'source' may not be empty here, either. Correct?
>
> If yes, please spell it out, and make sure the code catches it.
Yes, as above. It's checked in check_aliases():
if not a['source']:
raise QAPISemError(info, "'source' must not be empty")
> What if it resolve to a non-object? Is that an error? Is it caught?
Same as above, it just doesn't match.
> Continuing the double-checking... Say we have
>
> # alias missing
> 'source': ['gnu']
>
> where 'gnu' is an object with a member 'gnat'.
>
> Input "gnat": GNATVAL gets interpreted like "gnu": {"gnat": GNATVAL}.
>
> Correct?
Yes.
> The document could use examples. Feel free to steal mine.
>
> I think we should talk about 'alias' first, and only then about
> 'source'. It matches their order in the schema, and also matches how I
> think about aliases, namely "this name actually means that". Here,
> "this name" is 'alias', and "that" is 'source'.
>
> > +
> > +
>
> Don't get deceived by my comments; this is a pretty good start.
>
> I wish I had studied this part before PATCH 1.
>
> > === Documentation comments ===
> >
> > A multi-line comment that starts and ends with a '##' line is a
>
> I intend to look at the remainder shortly.
Ok. I'll prepare for a context switch to actually be able to address
your comments on the other patches and to figure out what I had already
addressed in my branch during your last review attempt.
I thought I had done a better than average job on documenting the code
(at least compare to my other patches), but doesn't seem so...
Kevin
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 6/6] tests/qapi-schema: Test cases for aliases
2020-11-12 17:28 ` [PATCH 6/6] tests/qapi-schema: Test cases " Kevin Wolf
@ 2021-02-10 13:09 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2021-02-10 13:09 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
I like to add tests in the same commit as the code they test, because
I feel it makes review slightly easier. Keeping them separate is not
wrong, of course.
Kevin Wolf <kwolf@redhat.com> writes:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
[...]
> diff --git a/tests/qapi-schema/alias-bad-type.err b/tests/qapi-schema/alias-bad-type.err
> new file mode 100644
> index 0000000000..a982d380b8
> --- /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' entries must be objects
Nitpick: we use "members" elsewhere, not "entries".
> 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..1cfe8a6aa5
> --- /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: alias misses key 'source'
Recommend
'aliases' member misses ...
for consistency with features-member-missing-name.err and the like.
> diff --git a/tests/qapi-schema/alias-missing-source.json b/tests/qapi-schema/alias-missing-source.json
> new file mode 100644
> index 0000000000..4ae22c2221
> --- /dev/null
> +++ b/tests/qapi-schema/alias-missing-source.json
> @@ -0,0 +1,3 @@
> +{ 'struct': 'AliasStruct0',
> + 'data': { 'foo': 'int' },
> + 'aliases': [ { 'alias': '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..83b9fe0b65
> --- /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 'alias' requires a string name
Would "'aliases' member 'alias'..." be more consistent?
> 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..6d993be332
> --- /dev/null
> +++ b/tests/qapi-schema/alias-name-bad-type.json
> @@ -0,0 +1,3 @@
> +{ 'struct': 'AliasStruct0',
> + 'data': { 'foo': 'int' },
> + 'aliases': [ { 'alias': ['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-source-bad-type.err b/tests/qapi-schema/alias-source-bad-type.err
> new file mode 100644
> index 0000000000..e3b949ff77
> --- /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: 'source' must be an array
Confusing when the struct also has a member 'source'. Recommend to
avoid this the same way as in the previous test's error.
> 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..0099a6439e
> --- /dev/null
> +++ b/tests/qapi-schema/alias-source-bad-type.json
> @@ -0,0 +1,3 @@
> +{ 'struct': 'AliasStruct0',
> + 'data': { 'foo': 'int' },
> + 'aliases': [ { 'alias': '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..44d219e352
> --- /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: element of alias member 'source' requires a string name
Nitpick: we use "member" elsewhere, not "element".
I think the error message could be better, but it's what we can get out
of check_name_is_str(). Let's not worry about this now.
> 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..28cb1c37c5
> --- /dev/null
> +++ b/tests/qapi-schema/alias-source-elem-bad-type.json
> @@ -0,0 +1,3 @@
> +{ 'struct': 'AliasStruct0',
> + 'data': { 'foo': 'int' },
> + 'aliases': [ { 'alias': '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..f7687e404c
> --- /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: 'source' must not be empty
Same comment as for alias-source-bad-type.err
> diff --git a/tests/qapi-schema/alias-source-empty.json b/tests/qapi-schema/alias-source-empty.json
> new file mode 100644
> index 0000000000..9d2d2f109f
> --- /dev/null
> +++ b/tests/qapi-schema/alias-source-empty.json
> @@ -0,0 +1,3 @@
> +{ 'struct': 'AliasStruct0',
> + 'data': { 'foo': 'int' },
> + 'aliases': [ { 'alias': '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-unknown-key.err b/tests/qapi-schema/alias-unknown-key.err
> new file mode 100644
> index 0000000000..d4dc5e4611
> --- /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: alias has unknown key 'known'
> +Valid keys are 'alias', 'source'.
Recommend
'aliases' member has ...
for consistency with features-member-unknown-key.err and the like.
> diff --git a/tests/qapi-schema/alias-unknown-key.json b/tests/qapi-schema/alias-unknown-key.json
> new file mode 100644
> index 0000000000..444f80ca30
> --- /dev/null
> +++ b/tests/qapi-schema/alias-unknown-key.json
> @@ -0,0 +1,3 @@
> +{ 'struct': 'AliasStruct0',
> + 'data': { 'foo': 'int' },
> + 'aliases': [ { 'alias': '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 304ef939bd..710cd60b61 100644
> --- a/tests/qapi-schema/meson.build
> +++ b/tests/qapi-schema/meson.build
> @@ -3,6 +3,14 @@ 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-source-bad-type.json',
> + 'alias-source-elem-bad-type.json',
> + 'alias-source-empty.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 63f92adf68..cc2497b2a2 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -325,3 +325,27 @@
>
> { 'event': 'TEST-EVENT-FEATURES1',
> 'features': [ 'deprecated' ] }
> +
> +# test 'aliases'
> +
> +{ 'struct': 'AliasStruct0',
> + 'data': { 'foo': 'int' },
> + 'aliases': [] }
> +{ 'struct': 'AliasStruct1',
> + 'data': { 'foo': 'int' },
> + 'aliases': [ { 'alias': 'bar', 'source': ['foo'] } ] }
> +{ 'struct': 'AliasStruct2',
> + 'data': { 'nested': 'AliasStruct1' },
> + 'aliases': [ { 'alias': 'bar', 'source': ['nested', 'foo'] } ] }
> +{ 'struct': 'AliasStruct3',
> + 'data': { 'nested': 'AliasStruct1' },
> + 'aliases': [ { 'source': ['nested'] } ] }
> +
> +{ 'union': 'AliasFlatUnion',
> + 'base': { 'tag': 'FeatureEnum1' },
> + 'discriminator': 'tag',
> + 'data': { 'eins': 'FeatureStruct1' },
> + 'aliases': [ { 'alias': 'bar', 'source': ['foo'] } ] }
> +{ 'union': 'AliasSimpleUnion',
> + 'data': { 'eins': 'AliasStruct1' },
> + 'aliases': [ { 'source': ['data'] } ] }
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 8868ca0dca..8ed88a257d 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -443,6 +443,35 @@ command test-command-cond-features3 None -> None
> 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 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/*
> + tag type
> + case eins: q_obj_AliasStruct1-wrapper
> module include/sub-module.json
> include sub-sub-module.json
> object SecondArrayRef
The negative tests should cover every error the previous commit adds.
Let's check:
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index e03abcbb95..6c94c01148 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -310,7 +310,7 @@ class QAPISchemaGenRSTVisitor(QAPISchemaVisitor):
+ self._nodes_for_if_section(ifcond))
def visit_object_type(self, name, info, ifcond, features,
- base, members, variants):
+ base, members, variants, aliases):
doc = self._cur_doc
if base and base.is_implicit():
base = None
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 2fcaaa2497..21fded529b 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -198,6 +198,32 @@ def check_features(features, info):
check_if(f, info, source)
+def check_aliases(aliases, info):
+ if aliases is None:
+ return
+ if not isinstance(aliases, list):
+ raise QAPISemError(info, "'aliases' must be an array")
aliases-bad-type
+ for a in aliases:
+ if not isinstance(a, dict):
+ raise QAPISemError(info, "'aliases' entries must be objects")
alias-bad-type
+ check_keys(a, info, "alias", ['source'], ['alias'])
alias-missing-source
alias-unknown-key
+
+ if 'alias' in a:
+ source = "alias member 'alias'"
+ check_name_is_str(a['alias'], info, source)
alias-name-bad-type
+ check_name_str(a['alias'], info, source)
Not covered. Okay; we don't have to cover it separately for every call
site.
+
+ if not isinstance(a['source'], list):
+ raise QAPISemError(info, "'source' must be an array")
alias-source-bad-type
+ if not a['source']:
+ raise QAPISemError(info, "'source' must not be empty")
alias-source-empty
+
+ source = "element of alias member 'source'"
+ for s in a['source']:
+ check_name_is_str(s, info, source)
alias-source-elem-bad-type
+ check_name_str(s, info, source)
Not covered. Okay; we don't have to cover it separately for every call
site.
+
+
def check_enum(expr, info):
name = expr['enum']
members = expr['data']
@@ -228,6 +254,7 @@ def check_struct(expr, info):
check_type(members, info, "'data'", allow_dict=name)
check_type(expr.get('base'), info, "'base'")
+ check_aliases(expr.get('aliases'), info)
def check_union(expr, info):
@@ -245,6 +272,8 @@ def check_union(expr, info):
raise QAPISemError(info, "'discriminator' requires 'base'")
check_name_is_str(discriminator, info, "'discriminator'")
+ check_aliases(expr.get('aliases'), info)
+
for (key, value) in members.items():
source = "'data' member '%s'" % key
check_name_str(key, info, source)
@@ -331,7 +360,7 @@ def check_exprs(exprs):
elif meta == 'union':
check_keys(expr, info, meta,
['union', 'data'],
- ['base', 'discriminator', 'if', 'features'])
+ ['base', 'discriminator', 'if', 'features', 'aliases'])
normalize_members(expr.get('base'))
normalize_members(expr['data'])
check_union(expr, info)
@@ -342,7 +371,8 @@ def check_exprs(exprs):
check_alternate(expr, info)
elif meta == 'struct':
check_keys(expr, info, meta,
- ['struct', 'data'], ['base', 'if', 'features'])
+ ['struct', 'data'],
+ ['base', 'if', 'features', 'aliases'])
normalize_members(expr['data'])
check_struct(expr, info)
elif meta == 'command':
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 720449feee..5daa137163 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -117,7 +117,7 @@ class QAPISchemaVisitor:
pass
def visit_object_type(self, name, info, ifcond, features,
- base, members, variants):
+ base, members, variants, aliases):
pass
def visit_object_type_flat(self, name, info, ifcond, features,
@@ -331,9 +331,16 @@ class QAPISchemaArrayType(QAPISchemaType):
return "%s type ['%s']" % (self.meta, self._element_type_name)
+class QAPISchemaAlias:
+ def __init__(self, alias, source):
+ assert source
+ self.alias = alias
+ self.source = source
No checking here, thus no errors to cover.
+
+
class QAPISchemaObjectType(QAPISchemaType):
def __init__(self, name, info, doc, ifcond, features,
- base, local_members, variants):
+ base, local_members, variants, aliases=None):
# struct has local_members, optional base, and no variants
# flat union has base, variants, and no local_members
# simple union has local_members, variants, and no base
@@ -351,6 +358,7 @@ class QAPISchemaObjectType(QAPISchemaType):
self.local_members = local_members
self.variants = variants
self.members = None
+ self.aliases = aliases or []
def check(self, schema):
# This calls another type T's .check() exactly when the C
@@ -443,7 +451,7 @@ class QAPISchemaObjectType(QAPISchemaType):
super().visit(visitor)
visitor.visit_object_type(
self.name, self.info, self.ifcond, self.features,
- self.base, self.local_members, self.variants)
+ self.base, self.local_members, self.variants, self.aliases)
visitor.visit_object_type_flat(
self.name, self.info, self.ifcond, self.features,
self.members, self.variants)
@@ -934,6 +942,12 @@ class QAPISchema:
return [QAPISchemaFeature(f['name'], info, f.get('if'))
for f in features]
+ def _make_aliases(self, aliases):
+ if aliases is None:
+ return []
+ return [QAPISchemaAlias(a.get('alias'), a['source'])
+ for a in aliases]
+
def _make_enum_members(self, values, info):
return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
for v in values]
@@ -1008,11 +1022,12 @@ class QAPISchema:
base = expr.get('base')
data = expr['data']
ifcond = expr.get('if')
+ aliases = self._make_aliases(expr.get('aliases'))
features = self._make_features(expr.get('features'), info)
self._def_entity(QAPISchemaObjectType(
name, info, doc, ifcond, features, base,
self._make_members(data, info),
- None))
+ None, aliases))
def _make_variant(self, case, typ, ifcond, info):
return QAPISchemaVariant(case, info, typ, ifcond)
@@ -1031,6 +1046,7 @@ class QAPISchema:
data = expr['data']
base = expr.get('base')
ifcond = expr.get('if')
+ aliases = self._make_aliases(expr.get('aliases'))
features = self._make_features(expr.get('features'), info)
tag_name = expr.get('discriminator')
tag_member = None
@@ -1055,7 +1071,8 @@ class QAPISchema:
QAPISchemaObjectType(name, info, doc, ifcond, features,
base, members,
QAPISchemaVariants(
- tag_name, info, tag_member, variants)))
+ tag_name, info, tag_member, variants),
+ aliases))
def _def_alternate_type(self, expr, info, doc):
name = expr['alternate']
Error coverage is okay.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] qapi: Add support for aliases
2021-02-10 12:26 ` Kevin Wolf
@ 2021-02-10 13:47 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2021-02-10 13:47 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, Markus Armbruster, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 10.02.2021 um 10:17 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Introduce alias definitions for object types (structs and unions). This
>> > allows using the same QAPI type and visitor for many syntax variations
>> > that exist in the external representation, like between QMP and the
>> > command line. It also provides a new tool for evolving the schema while
>> > maintaining backwards compatibility during a deprecation period.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> > docs/devel/qapi-code-gen.txt | 37 +++++++++++++++++++++++---
>> > docs/sphinx/qapidoc.py | 2 +-
>> > scripts/qapi/expr.py | 34 +++++++++++++++++++++--
>> > scripts/qapi/schema.py | 27 +++++++++++++++----
>> > scripts/qapi/types.py | 4 ++-
>> > scripts/qapi/visit.py | 33 ++++++++++++++++++++---
>> > tests/qapi-schema/test-qapi.py | 7 ++++-
>> > tests/qapi-schema/double-type.err | 2 +-
>> > tests/qapi-schema/unknown-expr-key.err | 2 +-
>> > 9 files changed, 130 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>> > index 6906a06ad2..6da14d5275 100644
>> > --- a/docs/devel/qapi-code-gen.txt
>> > +++ b/docs/devel/qapi-code-gen.txt
>> > @@ -231,7 +231,8 @@ Syntax:
>> > 'data': MEMBERS,
>> > '*base': STRING,
>> > '*if': COND,
>> > - '*features': FEATURES }
>> > + '*features': FEATURES,
>> > + '*aliases': ALIASES }
>> > MEMBERS = { MEMBER, ... }
>> > MEMBER = STRING : TYPE-REF
>> > | STRING : { 'type': TYPE-REF,
>>
>> Missing: a forward reference, like we have for 'if' and 'features'.
>> Here's the obvious one:
>>
>> The optional 'if' member specifies a conditional. See "Configuring
>> the schema" below for more on this.
>>
>> The optional 'features' member specifies features. See "Features"
>> below for more on this.
>>
>> +The optional 'aliases' member specifies aliases. See "Aliases" below
>> +for more on this.
>>
>> > @@ -286,13 +287,15 @@ Syntax:
>> > UNION = { 'union': STRING,
>> > 'data': BRANCHES,
>> > '*if': COND,
>> > - '*features': FEATURES }
>> > + '*features': FEATURES,
>> > + '*aliases': ALIASES }
>> > | { 'union': STRING,
>> > 'data': BRANCHES,
>> > 'base': ( MEMBERS | STRING ),
>> > 'discriminator': STRING,
>> > '*if': COND,
>> > - '*features': FEATURES }
>> > + '*features': FEATURES,
>> > + '*aliases': ALIASES }
>> > BRANCHES = { BRANCH, ... }
>> > BRANCH = STRING : TYPE-REF
>> > | STRING : { 'type': TYPE-REF, '*if': COND }
>>
>> Likewise.
>>
>> > @@ -837,6 +840,34 @@ shows a conditional entity only when the condition is satisfied in
>> > this particular build.
>> >
>> >
>> > +=== Aliases ===
>> > +
>> > +Object types, including structs and unions, can contain alias
>> > +definitions.
>> > +
>> > +Aliases define alternative member names that may be used in the
>> > +external representation to provide a value for a member in the same
>> > +object or in a nested object.
>>
>> "or one if its sub-objects"? Not sure which is better.
>
> "nested object" feels a little clearer to me, but not that it's a big
> difference. If you feel "sub-object" is better, I can use that.
>
>> > +
>> > +Syntax:
>> > + ALIAS = { '*alias': STRING,
>> > + 'source': [ STRING, ... ] }
>>
>> You used non-terminal ALIASES above. Please define it here.
>>
>> I have doubts about the name 'alias'. The alias is the complete thing,
>> and 'alias' is just one property of the complete thing. I think 'name'
>> would be better. Further evidence: below, you write "If 'alias' is
>> present" and "If 'alias' is not present". I think both read better with
>> 'name' instead of 'alias'.
>
> Works for me.
>
>> > +
>> > +'source' is a list of member names representing the path to an object
>> > +member, starting from the type where the alias definition is
>> > +specified.
>>
>> May 'source' be empty? More on that below.
>
> No. Empty 'source' isn't the path to any object member, so it doesn't
> meet the requirement. If you prefer, we can explicitly specify a
> "non-empty list".
I think it's best to be tediously explicit here.
>> "where the definition is specified" feels a bit awkward, and "path"
>> slightly hand-wavy. Let me try induction:
>>
>> 'source' is a list of member names. The first name is resolved in
>> the same object. Each subsequent member is resolved in the object
>> named by the preceding member.
>>
>> Differently awkward, I guess.
>
> Now you've left out what the purpose of it is. I think I'll combine your
> version with my first part ("'source' is a list of member names
> representing the path to an object member").
>
>> > It may refer to another alias name. It is allowed to use
>> > +a path that doesn't necessarily match an existing member in every
>> > +variant or even at all; in this case, the alias remains unused.
>>
>> Aha! Knowing this would've saved me some trouble in reviewing code.
>>
>> I wrote on PATCH 1:
>>
>> I think updating the big comment in visitor.h for aliases would help.
>> Let's postpone it until I've seen the rest of the series.
>>
>> We can cover unused aliases right there. Whether they also need to go
>> into contracts we'll see.
>
> Ok. I assume updating that big comment is still postponed because you
> saw the series, but didn't actually review all of it yet?
Writing documentation before I understand the code is probably not a
good use of my time, and my reviewer's time, too. Getting there.
If you want to try, go right ahead.
>> What if only a *prefix* of 'source' matches? E.g.
>>
>> 'source': ['eins', 'zwei', 'drei']
>>
>> and we have an object-valued member 'eins' (match), which has a member
>> 'zwei' (match), which is *not* an object. Is that an error? Is it
>> caught?
>
> This feels like a realistic case to me when 'eins' is a union type where
> some variants contain an object 'zwei' with a member 'drei' and others
> have 'zwei' as a non-object member.
>
> In this case, we want the alias not to match in the non-object 'zwei'
> case, but we do want it to match in another variant. So it is
> intentionally not an error.
>
> The QAPI generator could try to prove that there is at least one variant
> where the alias would actually be applied, but just leaving it unused
> when it doesn't match anywhere seemed good enough to me.
I see.
A typo can get your alias silently ignored. A bit of a trap. Testing
should catch this, of course.
Consider adding a comment in the QAPI generator along the lines "could
check this, but not sure it's worth our while", and a short note in
qapi-code-gen.txt to warn users about the trap.
>> > +
>> > +If 'alias' is present, then the single member referred to by 'source'
>> > +is made accessible with the name given in 'alias' in the type where
>> > +the alias definition is specified.
>>
>> 'source' may not be empty here. Correct?
>>
>> If yes, please spell it out.
>
> Yes. Does spelling it out more explicitly in the description of 'source'
> suffice?
I think so, yes.
>> Double-checking I got it... Say we have
>>
>> 'alias': 'foo',
>> 'source': ['bar', 'baz']
>>
>> where 'bar' is an object with a member 'baz'.
>>
>> Then input "foo": FOOVAL gets interpreted like "bar": {"baz": FOOVAL}.
>>
>> If input also contains "bar", we merge. Duplicates are an error.
>>
>> This is the case even when 'baz' is an object. If you want to alias
>> member 'foo' of 'baz', you have to say
>>
>> 'alias': 'foo',
>> 'source': ['bar', 'baz', 'foo']
>>
>> Correct?
>
> Correct.
>
>> > +
>> > +If 'alias' is not present, then all members in the object referred to
>> > +by 'source' are made accessible in the type where the alias definition
>> > +is specified with the same name as they have in 'source'.
>>
>> 'source' may not be empty here, either. Correct?
>>
>> If yes, please spell it out, and make sure the code catches it.
>
> Yes, as above. It's checked in check_aliases():
>
> if not a['source']:
> raise QAPISemError(info, "'source' must not be empty")
>
>> What if it resolve to a non-object? Is that an error? Is it caught?
>
> Same as above, it just doesn't match.
>
>> Continuing the double-checking... Say we have
>>
>> # alias missing
>> 'source': ['gnu']
>>
>> where 'gnu' is an object with a member 'gnat'.
>>
>> Input "gnat": GNATVAL gets interpreted like "gnu": {"gnat": GNATVAL}.
>>
>> Correct?
>
> Yes.
>
>> The document could use examples. Feel free to steal mine.
>>
>> I think we should talk about 'alias' first, and only then about
>> 'source'. It matches their order in the schema, and also matches how I
>> think about aliases, namely "this name actually means that". Here,
>> "this name" is 'alias', and "that" is 'source'.
>>
>> > +
>> > +
>>
>> Don't get deceived by my comments; this is a pretty good start.
>>
>> I wish I had studied this part before PATCH 1.
>>
>> > === Documentation comments ===
>> >
>> > A multi-line comment that starts and ends with a '##' line is a
>>
>> I intend to look at the remainder shortly.
>
> Ok. I'll prepare for a context switch to actually be able to address
> your comments on the other patches and to figure out what I had already
> addressed in my branch during your last review attempt.
I intend to look at the remainder of PATCH 5 this afternoon.
> I thought I had done a better than average job on documenting the code
> (at least compare to my other patches), but doesn't seem so...
Writing excellent documentation for code you just wrote is *hard*! I
think yours was pretty good, actually.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 5/6] qapi: Add support for aliases
2020-11-12 17:28 ` [PATCH 5/6] qapi: Add support for aliases Kevin Wolf
2020-11-12 18:34 ` Eric Blake
2021-02-10 9:17 ` Markus Armbruster
@ 2021-02-10 14:29 ` Markus Armbruster
2 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2021-02-10 14:29 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Introduce alias definitions for object types (structs and unions). This
> allows using the same QAPI type and visitor for many syntax variations
> that exist in the external representation, like between QMP and the
> command line. It also provides a new tool for evolving the schema while
> maintaining backwards compatibility during a deprecation period.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
[...]
> A multi-line comment that starts and ends with a '##' line is a
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index e03abcbb95..6c94c01148 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -310,7 +310,7 @@ class QAPISchemaGenRSTVisitor(QAPISchemaVisitor):
> + self._nodes_for_if_section(ifcond))
>
> def visit_object_type(self, name, info, ifcond, features,
> - base, members, variants):
> + base, members, variants, aliases):
> doc = self._cur_doc
> if base and base.is_implicit():
> base = None
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 2fcaaa2497..21fded529b 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -198,6 +198,32 @@ def check_features(features, info):
> check_if(f, info, source)
>
>
> +def check_aliases(aliases, info):
> + if aliases is None:
> + return
> + if not isinstance(aliases, list):
> + raise QAPISemError(info, "'aliases' must be an array")
> + for a in aliases:
> + if not isinstance(a, dict):
> + raise QAPISemError(info, "'aliases' entries must be objects")
> + check_keys(a, info, "alias", ['source'], ['alias'])
> +
> + if 'alias' in a:
> + source = "alias member 'alias'"
> + check_name_is_str(a['alias'], info, source)
> + check_name_str(a['alias'], info, source)
> +
> + if not isinstance(a['source'], list):
> + raise QAPISemError(info, "'source' must be an array")
> + if not a['source']:
> + raise QAPISemError(info, "'source' must not be empty")
> +
> + source = "element of alias member 'source'"
> + for s in a['source']:
> + check_name_is_str(s, info, source)
> + check_name_str(s, info, source)
> +
> +
> def check_enum(expr, info):
> name = expr['enum']
> members = expr['data']
> @@ -228,6 +254,7 @@ def check_struct(expr, info):
>
> check_type(members, info, "'data'", allow_dict=name)
> check_type(expr.get('base'), info, "'base'")
> + check_aliases(expr.get('aliases'), info)
>
>
> def check_union(expr, info):
> @@ -245,6 +272,8 @@ def check_union(expr, info):
> raise QAPISemError(info, "'discriminator' requires 'base'")
> check_name_is_str(discriminator, info, "'discriminator'")
>
> + check_aliases(expr.get('aliases'), info)
> +
> for (key, value) in members.items():
> source = "'data' member '%s'" % key
> check_name_str(key, info, source)
> @@ -331,7 +360,7 @@ def check_exprs(exprs):
> elif meta == 'union':
> check_keys(expr, info, meta,
> ['union', 'data'],
> - ['base', 'discriminator', 'if', 'features'])
> + ['base', 'discriminator', 'if', 'features', 'aliases'])
> normalize_members(expr.get('base'))
> normalize_members(expr['data'])
> check_union(expr, info)
> @@ -342,7 +371,8 @@ def check_exprs(exprs):
> check_alternate(expr, info)
> elif meta == 'struct':
> check_keys(expr, info, meta,
> - ['struct', 'data'], ['base', 'if', 'features'])
> + ['struct', 'data'],
> + ['base', 'if', 'features', 'aliases'])
> normalize_members(expr['data'])
> check_struct(expr, info)
> elif meta == 'command':
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 720449feee..5daa137163 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -117,7 +117,7 @@ class QAPISchemaVisitor:
> pass
>
> def visit_object_type(self, name, info, ifcond, features,
> - base, members, variants):
> + base, members, variants, aliases):
> pass
>
> def visit_object_type_flat(self, name, info, ifcond, features,
> @@ -331,9 +331,16 @@ class QAPISchemaArrayType(QAPISchemaType):
> return "%s type ['%s']" % (self.meta, self._element_type_name)
>
>
> +class QAPISchemaAlias:
> + def __init__(self, alias, source):
> + assert source
> + self.alias = alias
> + self.source = source
The existing QAPISchemaFOO.__init__() assert the arguments are sane. I
expect John's type hinting work to replace these assertions. Adding
type hints is much easier when you have assertions to replace. Let's
add them here, too.
> +
> +
> class QAPISchemaObjectType(QAPISchemaType):
> def __init__(self, name, info, doc, ifcond, features,
> - base, local_members, variants):
> + base, local_members, variants, aliases=None):
> # struct has local_members, optional base, and no variants
> # flat union has base, variants, and no local_members
> # simple union has local_members, variants, and no base
> @@ -351,6 +358,7 @@ class QAPISchemaObjectType(QAPISchemaType):
> self.local_members = local_members
> self.variants = variants
> self.members = None
> + self.aliases = aliases or []
>
> def check(self, schema):
> # This calls another type T's .check() exactly when the C
> @@ -443,7 +451,7 @@ class QAPISchemaObjectType(QAPISchemaType):
> super().visit(visitor)
> visitor.visit_object_type(
> self.name, self.info, self.ifcond, self.features,
> - self.base, self.local_members, self.variants)
> + self.base, self.local_members, self.variants, self.aliases)
> visitor.visit_object_type_flat(
> self.name, self.info, self.ifcond, self.features,
> self.members, self.variants)
> @@ -934,6 +942,12 @@ class QAPISchema:
> return [QAPISchemaFeature(f['name'], info, f.get('if'))
> for f in features]
>
> + def _make_aliases(self, aliases):
> + if aliases is None:
> + return []
> + return [QAPISchemaAlias(a.get('alias'), a['source'])
> + for a in aliases]
> +
> def _make_enum_members(self, values, info):
> return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
> for v in values]
> @@ -1008,11 +1022,12 @@ class QAPISchema:
> base = expr.get('base')
> data = expr['data']
> ifcond = expr.get('if')
> + aliases = self._make_aliases(expr.get('aliases'))
> features = self._make_features(expr.get('features'), info)
> self._def_entity(QAPISchemaObjectType(
> name, info, doc, ifcond, features, base,
> self._make_members(data, info),
> - None))
> + None, aliases))
>
> def _make_variant(self, case, typ, ifcond, info):
> return QAPISchemaVariant(case, info, typ, ifcond)
> @@ -1031,6 +1046,7 @@ class QAPISchema:
> data = expr['data']
> base = expr.get('base')
> ifcond = expr.get('if')
> + aliases = self._make_aliases(expr.get('aliases'))
> features = self._make_features(expr.get('features'), info)
> tag_name = expr.get('discriminator')
> tag_member = None
> @@ -1055,7 +1071,8 @@ class QAPISchema:
> QAPISchemaObjectType(name, info, doc, ifcond, features,
> base, members,
> QAPISchemaVariants(
> - tag_name, info, tag_member, variants)))
> + tag_name, info, tag_member, variants),
> + aliases))
>
> def _def_alternate_type(self, expr, info, doc):
> name = expr['alternate']
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 2b4916cdaa..e870bebb7e 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -25,6 +25,7 @@ from .common import (
> from .gen import QAPISchemaModularCVisitor, ifcontext
> from .schema import (
> QAPISchema,
> + QAPISchemaAlias,
> QAPISchemaEnumMember,
> QAPISchemaFeature,
> QAPISchemaObjectType,
> @@ -332,7 +333,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
> features: List[QAPISchemaFeature],
> base: Optional[QAPISchemaObjectType],
> members: List[QAPISchemaObjectTypeMember],
> - variants: Optional[QAPISchemaVariants]) -> None:
> + variants: Optional[QAPISchemaVariants],
> + aliases: List[QAPISchemaAlias]) -> None:
> # Nothing to do for the special empty builtin
> if name == 'q_empty':
> return
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 339f152152..a35921ef2c 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -26,6 +26,7 @@ from .common import (
> from .gen import QAPISchemaModularCVisitor, ifcontext
> from .schema import (
> QAPISchema,
> + QAPISchemaAlias,
> QAPISchemaEnumMember,
> QAPISchemaEnumType,
> QAPISchemaFeature,
> @@ -60,7 +61,8 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
> def gen_visit_object_members(name: str,
> base: Optional[QAPISchemaObjectType],
> members: List[QAPISchemaObjectTypeMember],
> - variants: Optional[QAPISchemaVariants]) -> str:
> + variants: Optional[QAPISchemaVariants],
> + aliases: List[QAPISchemaAlias]) -> str:
> ret = mcgen('''
>
> bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
> @@ -68,6 +70,25 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
> ''',
> c_name=c_name(name))
>
> + if aliases:
> + ret += mcgen('''
> + visit_start_alias_scope(v);
> +''')
> +
> + for a in aliases:
> + if a.alias:
> + alias = '"%s"' % a.alias
> + else:
> + alias = "NULL"
> +
> + source_list = ", ".join('"%s"' % x for x in a.source)
> + source = "(const char * []) { %s, NULL }" % source_list
> +
> + ret += mcgen('''
> + visit_define_alias(v, %(alias)s, %(source)s);
> +''',
> + alias=alias, source=source)
> +
I prefer putting C code into mcgen() as much as practical. Like this:
if a.alias:
alias = '"%s"' % a.alias
else:
alias = "NULL"
source = ", ".join('"%s"' % x for x in a.source)
ret += mcgen('''
visit_define_alias(v, %(alias)s, (const char * []){ %(source), NULL });
''',
alias=alias, source=source)
> if base:
> ret += mcgen('''
> if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
> @@ -133,6 +154,11 @@ bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
> }
> ''')
>
> + if aliases:
> + ret += mcgen('''
> + visit_end_alias_scope(v);
> +''')
> +
> ret += mcgen('''
> return true;
> }
> @@ -361,14 +387,15 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
> features: List[QAPISchemaFeature],
> base: Optional[QAPISchemaObjectType],
> members: List[QAPISchemaObjectTypeMember],
> - variants: Optional[QAPISchemaVariants]) -> None:
> + variants: Optional[QAPISchemaVariants],
> + aliases: List[QAPISchemaAlias]) -> None:
> # Nothing to do for the special empty builtin
> if name == 'q_empty':
> return
> with ifcontext(ifcond, self._genh, self._genc):
> self._genh.add(gen_visit_members_decl(name))
> self._genc.add(gen_visit_object_members(name, base,
> - members, variants))
> + members, variants, aliases))
Long line. Recommend hanging indent:
self._genc.add(gen_visit_object_members(
name, base, members, variants, aliases))
> # TODO Worth changing the visitor signature, so we could
> # directly use rather than repeat type.is_implicit()?
> if not name.startswith('q_'):
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index e8db9d09d9..adf8bda89a 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -47,7 +47,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
> self._print_if(ifcond)
>
> def visit_object_type(self, name, info, ifcond, features,
> - base, members, variants):
> + base, members, variants, aliases):
> print('object %s' % name)
> if base:
> print(' base %s' % base.name)
> @@ -56,6 +56,11 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
> % (m.name, m.type.name, m.optional))
> self._print_if(m.ifcond, 8)
> self._print_features(m.features, indent=8)
> + for a in aliases:
> + if a.alias:
> + print(' alias %s -> %s' % (a.alias, '/'.join(a.source)))
> + else:
> + print(' alias * -> %s/*' % '/'.join(a.source))
For better or worse, we use '.' as a separator elsewhere.
> self._print_variants(variants)
> self._print_if(ifcond)
> self._print_features(features)
> diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err
> index 71fc4dbb52..5d25d7623c 100644
> --- a/tests/qapi-schema/double-type.err
> +++ b/tests/qapi-schema/double-type.err
> @@ -1,3 +1,3 @@
> double-type.json: In struct 'bar':
> double-type.json:2: struct has unknown key 'command'
> -Valid keys are 'base', 'data', 'features', 'if', 'struct'.
> +Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'.
> diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err
> index c5f395bf79..7429d1ff03 100644
> --- a/tests/qapi-schema/unknown-expr-key.err
> +++ b/tests/qapi-schema/unknown-expr-key.err
> @@ -1,3 +1,3 @@
> unknown-expr-key.json: In struct 'bar':
> unknown-expr-key.json:2: struct has unknown keys 'bogus', 'phony'
> -Valid keys are 'base', 'data', 'features', 'if', 'struct'.
> +Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'.
Straightforward, easy to review. Appreciated!
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 0/6] qapi: Add support for aliases
2020-11-12 17:28 [PATCH 0/6] qapi: Add support for aliases Kevin Wolf
` (6 preceding siblings ...)
2020-12-04 9:46 ` [PATCH 0/6] qapi: Add support " Kevin Wolf
@ 2021-02-10 14:38 ` Markus Armbruster
7 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2021-02-10 14:38 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
I'm done for now. I think there's enough material (and also enough
promise!) to justify a respin. I'll do my best to review it promptly.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor
2021-02-09 12:57 ` Markus Armbruster
@ 2021-02-11 16:27 ` Kevin Wolf
0 siblings, 0 replies; 32+ messages in thread
From: Kevin Wolf @ 2021-02-11 16:27 UTC (permalink / raw)
To: Markus Armbruster; +Cc: jsnow, qemu-devel
Am 09.02.2021 um 13:57 hat Markus Armbruster geschrieben:
> Let me look at the actual code now Kevin reduced my confusion about the
> interface and the data structures.
>
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > This makes qobject-input-visitor remember the currently valid aliases in
> > each StackObject. It doesn't actually allow using the aliases yet.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 115 insertions(+)
> > @@ -203,6 +229,38 @@ static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv,
> > return qstring_get_str(qstr);
> > }
> >
> > +/*
> > + * Propagates aliases from the parent StackObject @src to its direct
> > + * child StackObject @dst, which is representing the child struct @name.
>
> @name must equal dst->name, I think. Drop the parameter?
>
> > + *
> > + * Every alias whose source path begins with @name and which still
> > + * applies in @dst (i.e. it is either a wildcard alias or has at least
> > + * one more source path element) is propagated to @dst with the first
>
> I'm not sure I get the parenthesis. Perhaps the code will enlighten me.
>
> > + * element (i.e. @name) removed from the source path.
> > + */
> > +static void propagate_aliases(StackObject *dst, StackObject *src,
> > + const char *name)
> > +{
> > + InputVisitorAlias *a;
> > +
> > + QSLIST_FOREACH(a, &src->aliases, next) {
> > + if (!a->src[0] || strcmp(a->src[0], name)) {
> > + continue;
> > + }
>
> We only look at the aliases that apply to @dst or below. They do only
> when ->src[0] equals dst->name. Makes sense.
>
> > + if (a->src[1] || !a->alias) {
>
> If a->src[1], the alias applies below @dst, not to @dst. To get it to
> the place where it applies, we first need to propagate to @dst.
>
> Else, the alias applies to @dst. If !a->alias, we're looking at a
> wildcard alias, i.e. all members of the object described by @dst are
> aliased. Why do we need to propagate only wildcard aliases to @dst?
If it's not a wildcard alias and a->src[1] == NULL, then the alias
referred to @dst's name inside @src. It's not valid inside @dst any
more.
This is what the parenthesis above tried to tell you.
I've added another comment in the code to explain this case more
explicitly.
> > + InputVisitorAlias *alias = g_new(InputVisitorAlias, 1);
> > +
> > + *alias = (InputVisitorAlias) {
> > + .alias = a->alias,
> > + .alias_so = a->alias_so,
> > + .src = &a->src[1],
> > + };
> > +
> > + QSLIST_INSERT_HEAD(&dst->aliases, alias, next);
> > + }
> > + }
> > +}
> > +
> > static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
> > const char *name,
> > QObject *obj, void *qapi)
> > @@ -226,6 +284,9 @@ static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
> > g_hash_table_insert(h, (void *)qdict_entry_key(entry), NULL);
> > }
> > tos->h = h;
> > + if (!QSLIST_EMPTY(&qiv->stack)) {
> > + propagate_aliases(tos, QSLIST_FIRST(&qiv->stack), name);
> > + }
>
> What if QSLIST_EMPTY(&qiv->stack) and tos->aliases contains aliases that
> apply deeper?
tos->aliases doesn't contain any aliases, we only created it a few lines
above.
We would normally propagate aliases from the parent StackObject (which
is QSLIST_FIRST(&qiv->stack)), but if there is no parent StackObject,
then there can't be aliases to be inherited from the parent either.
Kevin
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2021-02-11 16:31 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-12 17:28 [PATCH 0/6] qapi: Add support for aliases Kevin Wolf
2020-11-12 17:28 ` [PATCH 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
2021-01-26 15:59 ` Markus Armbruster
2021-01-27 12:51 ` Markus Armbruster
2021-01-27 20:31 ` Kevin Wolf
2021-02-02 13:51 ` Markus Armbruster
2020-11-12 17:28 ` [PATCH 2/6] qapi: Remember alias definitions in qobject-input-visitor Kevin Wolf
2021-01-27 13:06 ` Markus Armbruster
2021-01-27 20:59 ` Kevin Wolf
2021-02-09 12:55 ` Markus Armbruster
2021-02-09 12:57 ` Markus Armbruster
2021-02-11 16:27 ` Kevin Wolf
2020-11-12 17:28 ` [PATCH 3/6] qapi: Simplify full_name_nth() " Kevin Wolf
2021-01-27 13:56 ` Markus Armbruster
2021-01-27 21:42 ` Kevin Wolf
2021-01-28 7:43 ` Markus Armbruster
2021-01-28 10:57 ` Kevin Wolf
2020-11-12 17:28 ` [PATCH 4/6] qapi: Apply aliases " Kevin Wolf
2021-02-09 16:02 ` Markus Armbruster
2020-11-12 17:28 ` [PATCH 5/6] qapi: Add support for aliases Kevin Wolf
2020-11-12 18:34 ` Eric Blake
2020-11-13 9:46 ` Kevin Wolf
2020-11-20 14:41 ` Peter Krempa
2020-11-20 15:06 ` Daniel P. Berrangé
2021-02-10 9:17 ` Markus Armbruster
2021-02-10 12:26 ` Kevin Wolf
2021-02-10 13:47 ` Markus Armbruster
2021-02-10 14:29 ` Markus Armbruster
2020-11-12 17:28 ` [PATCH 6/6] tests/qapi-schema: Test cases " Kevin Wolf
2021-02-10 13:09 ` Markus Armbruster
2020-12-04 9:46 ` [PATCH 0/6] qapi: Add support " Kevin Wolf
2021-02-10 14:38 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.