* [PATCH v2 0/6] qapi: Add support for aliases
@ 2021-02-11 18:31 Kevin Wolf
2021-02-11 18:31 ` [PATCH v2 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
` (6 more replies)
0 siblings, 7 replies; 27+ messages in thread
From: Kevin Wolf @ 2021-02-11 18:31 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-v2
v2:
- Renamed 'alias' to 'name' in all data structures describing aliases
- Tons of new or changed comments and other documentation
- Be more explicit that empty 'source' is invalid and assert it
- Fixed full_name_so() for lists (added a parameter to tell the function
whether the name of a list member or the list itself is meant)
- Changed some QAPI generator error messages
- Assert the type of parameters in QAPISchemaAlias.__init__()
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 | 105 ++++-
docs/sphinx/qapidoc.py | 2 +-
include/qapi/visitor-impl.h | 12 +
include/qapi/visitor.h | 44 ++
qapi/qapi-visit-core.c | 22 +
qapi/qobject-input-visitor.c | 402 ++++++++++++++++--
scripts/qapi/expr.py | 36 +-
scripts/qapi/schema.py | 30 +-
scripts/qapi/types.py | 4 +-
scripts/qapi/visit.py | 34 +-
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, 757 insertions(+), 47 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.29.2
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/6] qapi: Add interfaces for alias support to Visitor
2021-02-11 18:31 [PATCH v2 0/6] qapi: Add support for aliases Kevin Wolf
@ 2021-02-11 18:31 ` Kevin Wolf
2021-02-16 11:56 ` Markus Armbruster
2021-02-11 18:31 ` [PATCH v2 2/6] qapi: Remember alias definitions in qobject-input-visitor Kevin Wolf
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2021-02-11 18:31 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 | 44 +++++++++++++++++++++++++++++++++++++
qapi/qapi-visit-core.c | 22 +++++++++++++++++++
3 files changed, 78 insertions(+)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7362c043be..d9a6874528 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 *name, 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..2ecbc20624 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -459,6 +459,50 @@ void visit_end_alternate(Visitor *v, void **obj);
*/
bool visit_optional(Visitor *v, const char *name, bool *present);
+/*
+ * Defines a new alias rule.
+ *
+ * If @name is non-NULL, the member called @name in the external
+ * representation of the currently visited object is defined as an
+ * alias for the member described by @source. It is not allowed to
+ * call this function when the currently visited type is not an
+ * object.
+ *
+ * If @name is NULL, all members of the object described by @source
+ * are considered to have alias members with the same key in the
+ * currently visited object.
+ *
+ * @source is a NULL-terminated non-empty array of names that describe
+ * the path to a member, starting from the currently visited object.
+ * All elements in @source except the last one should describe
+ * objects. If an intermediate element refers to a member with a
+ * non-object type, the alias won't work (this case can legitimately
+ * happen in unions where an alias only makes sense for one branch,
+ * but not for another).
+ *
+ * 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 *name, 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..651dd88e02 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -135,6 +135,28 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
return *present;
}
+void visit_define_alias(Visitor *v, const char *name, const char **source)
+{
+ assert(source[0] != NULL);
+ if (v->define_alias) {
+ v->define_alias(v, name, 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.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 2/6] qapi: Remember alias definitions in qobject-input-visitor
2021-02-11 18:31 [PATCH v2 0/6] qapi: Add support for aliases Kevin Wolf
2021-02-11 18:31 ` [PATCH v2 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
@ 2021-02-11 18:31 ` Kevin Wolf
2021-02-16 12:06 ` Markus Armbruster
2021-02-11 18:31 ` [PATCH v2 3/6] qapi: Simplify full_name_nth() " Kevin Wolf
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2021-02-11 18:31 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 | 145 +++++++++++++++++++++++++++++++++++
1 file changed, 145 insertions(+)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 23843b242e..aa95cd49bd 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -29,6 +29,50 @@
#include "qemu/cutils.h"
#include "qemu/option.h"
+/*
+ * Describes an alias that is relevant for the current StackObject,
+ * either because it aliases a member of the currently visited object
+ * or because it aliases a member of a nested object.
+ *
+ * When processing a nested object, all InputVisitorAlias objects that
+ * are relevant for the nested object are propagated, i.e. copied with
+ * the name of the nested object removed from @source.
+ */
+typedef struct InputVisitorAlias {
+ /* StackObject in which the alias was defined */
+ struct StackObject *alias_so;
+
+ /*
+ * Alias name as defined for @alias_so.
+ * NULL means that this is a wildcard alias, i.e. all members of
+ * @src get an alias in @alias_so with the same name.
+ */
+ const char *name;
+
+ /*
+ * NULL terminated array representing a path to the source member
+ * that the alias refers to.
+ *
+ * Must contain at least one non-NULL element if @alias is not NULL.
+ *
+ * If it contains no non-NULL element, @alias_so must be different
+ * from the StackObject which contains this InputVisitorAlias in
+ * its aliases list. In this case, all elements in the currently
+ * visited object have an alias with the same name in @alias_so.
+ */
+ const char **src;
+
+ /*
+ * The alias remains valid as long as the StackObject which
+ * contains this InputVisitorAlias in its aliases list 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 +82,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; /* Number of open alias scopes */
+
QSLIST_ENTRY(StackObject) node; /* parent */
} StackObject;
@@ -203,6 +250,43 @@ static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv,
return qstring_get_str(qstr);
}
+/*
+ * Propagate 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 @dst->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. @dst->name) removed from the source path.
+ */
+static void propagate_aliases(StackObject *dst, StackObject *src)
+{
+ InputVisitorAlias *a;
+
+ QSLIST_FOREACH(a, &src->aliases, next) {
+ if (!a->src[0] || strcmp(a->src[0], dst->name)) {
+ continue;
+ }
+
+ /*
+ * If this is not a wildcard alias, but a->src[1] is NULL,
+ * then it referred to dst->name in src and doesn't apply
+ * inside dst any more.
+ */
+ if (a->src[1] || !a->name) {
+ InputVisitorAlias *alias = g_new(InputVisitorAlias, 1);
+
+ *alias = (InputVisitorAlias) {
+ .name = a->name,
+ .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 +310,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));
+ }
} else {
assert(qlist);
tos->entry = qlist_first(qlist);
@@ -257,10 +344,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 +368,54 @@ 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 *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 become empty during alias propagation for
+ * wildcard aliases, but not when defining an alias (it would map
+ * all names onto themselves, which doesn't make sense).
+ */
+ assert(source[0]);
+
+ *alias = (InputVisitorAlias) {
+ .name = 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 +838,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.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor
2021-02-11 18:31 [PATCH v2 0/6] qapi: Add support for aliases Kevin Wolf
2021-02-11 18:31 ` [PATCH v2 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
2021-02-11 18:31 ` [PATCH v2 2/6] qapi: Remember alias definitions in qobject-input-visitor Kevin Wolf
@ 2021-02-11 18:31 ` Kevin Wolf
2021-02-16 12:22 ` Markus Armbruster
2021-02-11 18:31 ` [PATCH v2 4/6] qapi: Apply aliases " Kevin Wolf
` (3 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2021-02-11 18:31 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 | 43 ++++++++++++++++++++----------------
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index aa95cd49bd..dd04ef0027 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -108,20 +108,20 @@ 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 a member in @so which @qiv is currently
+ * visiting. If the currently visited thing is an object, @name is
+ * the (local) name of the member to describe. If it is a list, @name
+ * is ignored and the current index (so->index) is included.
+ *
+ * If @skip_member is true, find the full name of @so itself instead.
+ * @name must be NULL then.
+ *
+ * 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,
+ bool skip_member, StackObject *so)
{
- StackObject *so;
char buf[32];
if (qiv->errname) {
@@ -130,10 +130,14 @@ 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 (skip_member && so) {
+ assert(name == NULL);
+ 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 {
@@ -144,7 +148,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);
@@ -159,7 +162,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, false, tos);
}
static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
@@ -503,7 +508,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, true, tos));
return false;
}
return true;
--
2.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
2021-02-11 18:31 [PATCH v2 0/6] qapi: Add support for aliases Kevin Wolf
` (2 preceding siblings ...)
2021-02-11 18:31 ` [PATCH v2 3/6] qapi: Simplify full_name_nth() " Kevin Wolf
@ 2021-02-11 18:31 ` Kevin Wolf
2021-02-17 15:32 ` Markus Armbruster
` (2 more replies)
2021-02-11 18:31 ` [PATCH v2 5/6] qapi: Add support for aliases Kevin Wolf
` (2 subsequent siblings)
6 siblings, 3 replies; 27+ messages in thread
From: Kevin Wolf @ 2021-02-11 18:31 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 | 214 +++++++++++++++++++++++++++++++++--
1 file changed, 205 insertions(+), 9 deletions(-)
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index dd04ef0027..3ea5e5abd6 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -95,6 +95,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;
@@ -167,9 +169,178 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
return full_name_so(qiv, name, false, tos);
}
+static bool find_object_member(QObjectInputVisitor *qiv,
+ StackObject **so, const char **name,
+ bool *implicit_object, Error **errp);
+
+/*
+ * Check whether the alias member defined by @a is present in the
+ * input and can be used to obtain the value for the member @name in
+ * the currently visited object.
+ */
+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->name) {
+ name = a->name;
+ }
+
+ /*
+ * Check whether the alias member is present in the input
+ * (possibly recursively because aliases are transitive).
+ */
+ 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;
+}
+
+/*
+ * Check whether the member @name in the object visited by @so can be
+ * specified in the input by using the alias described by @a.
+ *
+ * If @name is only a prefix of the alias source, but doesn't match
+ * immediately, false is returned and @implicit_object is set to true
+ * if it is non-NULL. In all other cases, @implicit_object is left
+ * unchanged.
+ */
+static bool alias_source_matches(QObjectInputVisitor *qiv,
+ StackObject *so, InputVisitorAlias *a,
+ const char *name, bool *implicit_object)
+{
+ if (a->src[0] == NULL) {
+ assert(a->name == NULL);
+ return true;
+ }
+
+ if (!strcmp(a->src[0], name)) {
+ if (a->name && 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->name || alias_present(qiv, a, a->name)) {
+ *implicit_object = true;
+ }
+ }
+ }
+
+ return false;
+}
+
+/*
+ * Find the place in the input where the value for the object member
+ * @name in @so is specified, considering applicable aliases.
+ *
+ * If a value could be found, true is returned and @so and @name are
+ * updated to identify the key name and StackObject where the value
+ * can be found in the input. (This is either unchanged or the
+ * alias_so/name of an alias.) The value of @implicit_object on
+ * return is undefined in this case.
+ *
+ * If no value could be found in the input, false is returned. This
+ * is not an error and @errp remains unchanged. If @implicit_object
+ * is non-NULL, it is set to true if the given name is a prefix of the
+ * source path of an alias for which a value may be present in the
+ * input. It is set to false otherwise.
+ *
+ * If an error occurs (e.g. two values are specified for the member
+ * through different names), false is returned and @errp is set. The
+ * value of @implicit_object on return is undefined in this case.
+ */
+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->name from a->alias_so.
+ */
+ QSLIST_FOREACH(a, &cur_so->aliases, next) {
+ if (a->name == 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, false, *so));
+ return false;
+ } else {
+ found = a->name ?: *name;
+ *so = a->alias_so;
+ found_is_wildcard = !a->name;
+ }
+ }
+
+ /* 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;
@@ -187,10 +358,30 @@ 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) {
+ /*
+ * The member is not present in the input, but
+ * something inside of it might still be given through
+ * an alias. Pretend there was an empty object in the
+ * input.
+ */
+ 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 {
@@ -216,9 +407,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;
@@ -799,13 +991,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;
}
@@ -820,6 +1015,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.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 5/6] qapi: Add support for aliases
2021-02-11 18:31 [PATCH v2 0/6] qapi: Add support for aliases Kevin Wolf
` (3 preceding siblings ...)
2021-02-11 18:31 ` [PATCH v2 4/6] qapi: Apply aliases " Kevin Wolf
@ 2021-02-11 18:31 ` Kevin Wolf
2021-02-16 15:43 ` Markus Armbruster
2021-02-17 15:23 ` Markus Armbruster
2021-02-11 18:31 ` [PATCH v2 6/6] tests/qapi-schema: Test cases " Kevin Wolf
2021-02-24 8:45 ` [PATCH v2 0/6] qapi: Add support " Markus Armbruster
6 siblings, 2 replies; 27+ messages in thread
From: Kevin Wolf @ 2021-02-11 18:31 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 | 105 ++++++++++++++++++++++++-
docs/sphinx/qapidoc.py | 2 +-
scripts/qapi/expr.py | 36 ++++++++-
scripts/qapi/schema.py | 30 +++++--
scripts/qapi/types.py | 4 +-
scripts/qapi/visit.py | 34 +++++++-
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, 203 insertions(+), 19 deletions(-)
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 6906a06ad2..247c4b8ef4 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,
@@ -279,6 +280,9 @@ 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.
+
=== Union types ===
@@ -286,13 +290,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 }
@@ -402,6 +408,9 @@ 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.
+
=== Alternate types ===
@@ -837,6 +846,96 @@ 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:
+ ALIASES = [ ALIAS, ... ]
+ ALIAS = { '*name': STRING,
+ 'source': [ STRING, ... ] }
+
+If 'name' is present, then the single member referred to by 'source'
+is made accessible with the name given in 'name' in the type where the
+alias definition is specified.
+
+If 'name' 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' is a non-empty list of member names representing the path to
+an object member. The first name is resolved in the same object. Each
+subsequent member is resolved in the object named by the preceding
+member.
+
+Example: Alternative name for a member in the same object (the member
+"path" may be given through its alias "filename" in the external
+representation):
+
+{ 'struct': 'File',
+ 'data': { 'path': 'str' },
+ 'aliases': [ { 'name': 'filename', 'source': ['path'] } ] }
+
+Example: Alias for a member in a nested object:
+
+{ 'struct': 'A',
+ 'data': { 'zahl': 'int' } }
+{ 'struct': 'B',
+ 'data': { 'drei': 'A' } }
+{ 'struct': 'C',
+ 'data': { 'zwei': 'B' } }
+{ 'struct': 'D',
+ 'data': { 'eins': 'C' },
+ 'aliases': [ { 'name': 'number',
+ 'source': ['eins', 'zwei', 'drei', 'zahl' ] },
+ { 'name': 'the_B',
+ 'source': ['eins','zwei'] } ] }
+
+With this definition, each of the following inputs mean the same:
+
+* { 'eins': { 'zwei': { 'drei': { 'zahl': 42 } } } }
+
+* { 'the_B': { 'drei': { 'zahl': 42 } } }
+
+* { 'number': 42 }
+
+Example: Flattening a union with a wildcard alias that maps all
+members of 'data' to the top level:
+
+{ 'union': 'SocketAddress',
+ 'data': {
+ 'inet': 'InetSocketAddress',
+ 'unix': 'UnixSocketAddress' },
+ 'aliases': [ { 'source': ['data'] } ] }
+
+Aliases are transitive: 'source' may refer to another alias name. In
+this case, the alias is effectively an altenative name for the source
+of the other alias.
+
+Example: Giving "the_answer" on the top level provides a value for
+"zahl" in the nested object:
+
+{ 'struct': 'A',
+ 'data': { 'zahl': 'int' },
+ 'aliases': [ { 'name': 'number', 'source': ['zahl'] } ] }
+{ 'struct': 'B',
+ 'data': { 'nested': 'A' },
+ 'aliases': [ { 'name': 'the_answer',
+ 'source': ['nested', 'number'] } ] }
+
+In order to accommodate unions where variants differ in structure, 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. Note that the QAPI generator does not check whether
+there is at least one branch for which an alias could match. If a
+source member is misspelt, the alias just won't work.
+
+
=== 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..743e23ec85 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -198,6 +198,34 @@ 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' members must be objects")
+ check_keys(a, info, "'aliases' member", ['source'], ['name'])
+
+ if 'name' in a:
+ source = "alias member 'name'"
+ check_name_is_str(a['name'], info, source)
+ check_name_str(a['name'], info, source)
+
+ if not isinstance(a['source'], list):
+ raise QAPISemError(info,
+ "alias member 'source' must be an array")
+ if not a['source']:
+ raise QAPISemError(info,
+ "alias member 'source' must not be empty")
+
+ source = "member 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 +256,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 +274,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 +362,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 +373,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 353e8020a2..14a2b0175b 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -118,7 +118,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,
@@ -362,9 +362,19 @@ class QAPISchemaArrayType(QAPISchemaType):
return "%s type ['%s']" % (self.meta, self._element_type_name)
+class QAPISchemaAlias:
+ def __init__(self, name, source):
+ assert name is None or isinstance(name, str)
+ assert source
+ for member in source:
+ assert isinstance(member, str)
+ self.name = name
+ 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
@@ -382,6 +392,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
@@ -474,7 +485,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)
@@ -964,6 +975,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('name'), 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]
@@ -1038,11 +1055,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)
@@ -1061,6 +1079,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
@@ -1085,7 +1104,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 2bdd626847..c8306479f5 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 22e62df901..e370485f6e 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,24 @@ 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.name:
+ name = '"%s"' % a.name
+ else:
+ name = "NULL"
+
+ source = ", ".join('"%s"' % x for x in a.source)
+
+ ret += mcgen('''
+ visit_define_alias(v, %(name)s, (const char * []) { %(source)s, NULL });
+''',
+ name=name, source=source)
+
if base:
ret += mcgen('''
if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
@@ -133,6 +153,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 +386,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))
+ 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..1679d1b5da 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.name:
+ print(' alias %s -> %s' % (a.name, '.'.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.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 6/6] tests/qapi-schema: Test cases for aliases
2021-02-11 18:31 [PATCH v2 0/6] qapi: Add support for aliases Kevin Wolf
` (4 preceding siblings ...)
2021-02-11 18:31 ` [PATCH v2 5/6] qapi: Add support for aliases Kevin Wolf
@ 2021-02-11 18:31 ` Kevin Wolf
2021-02-16 15:14 ` Markus Armbruster
2021-02-24 8:45 ` [PATCH v2 0/6] qapi: Add support " Markus Armbruster
6 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2021-02-11 18:31 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..820e18ed9c
--- /dev/null
+++ b/tests/qapi-schema/alias-bad-type.err
@@ -0,0 +1,2 @@
+alias-bad-type.json: In struct 'AliasStruct0':
+alias-bad-type.json:1: 'aliases' members must be objects
diff --git a/tests/qapi-schema/alias-bad-type.json b/tests/qapi-schema/alias-bad-type.json
new file mode 100644
index 0000000000..0aa5d206fe
--- /dev/null
+++ b/tests/qapi-schema/alias-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ 'must be an object' ] }
diff --git a/tests/qapi-schema/alias-bad-type.out b/tests/qapi-schema/alias-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-missing-source.err b/tests/qapi-schema/alias-missing-source.err
new file mode 100644
index 0000000000..8b7d601fbf
--- /dev/null
+++ b/tests/qapi-schema/alias-missing-source.err
@@ -0,0 +1,2 @@
+alias-missing-source.json: In struct 'AliasStruct0':
+alias-missing-source.json:1: 'aliases' member misses key 'source'
diff --git a/tests/qapi-schema/alias-missing-source.json b/tests/qapi-schema/alias-missing-source.json
new file mode 100644
index 0000000000..b6c91a9488
--- /dev/null
+++ b/tests/qapi-schema/alias-missing-source.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar' } ] }
diff --git a/tests/qapi-schema/alias-missing-source.out b/tests/qapi-schema/alias-missing-source.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-name-bad-type.err b/tests/qapi-schema/alias-name-bad-type.err
new file mode 100644
index 0000000000..489f45ff9b
--- /dev/null
+++ b/tests/qapi-schema/alias-name-bad-type.err
@@ -0,0 +1,2 @@
+alias-name-bad-type.json: In struct 'AliasStruct0':
+alias-name-bad-type.json:1: alias member 'name' requires a string name
diff --git a/tests/qapi-schema/alias-name-bad-type.json b/tests/qapi-schema/alias-name-bad-type.json
new file mode 100644
index 0000000000..17442d5939
--- /dev/null
+++ b/tests/qapi-schema/alias-name-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': ['bar'], 'source': ['foo'] } ] }
diff --git a/tests/qapi-schema/alias-name-bad-type.out b/tests/qapi-schema/alias-name-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-bad-type.err b/tests/qapi-schema/alias-source-bad-type.err
new file mode 100644
index 0000000000..b1779cbb8e
--- /dev/null
+++ b/tests/qapi-schema/alias-source-bad-type.err
@@ -0,0 +1,2 @@
+alias-source-bad-type.json: In struct 'AliasStruct0':
+alias-source-bad-type.json:1: alias member 'source' must be an array
diff --git a/tests/qapi-schema/alias-source-bad-type.json b/tests/qapi-schema/alias-source-bad-type.json
new file mode 100644
index 0000000000..d6a7430ee3
--- /dev/null
+++ b/tests/qapi-schema/alias-source-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': 'foo' } ] }
diff --git a/tests/qapi-schema/alias-source-bad-type.out b/tests/qapi-schema/alias-source-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-elem-bad-type.err b/tests/qapi-schema/alias-source-elem-bad-type.err
new file mode 100644
index 0000000000..f73fbece77
--- /dev/null
+++ b/tests/qapi-schema/alias-source-elem-bad-type.err
@@ -0,0 +1,2 @@
+alias-source-elem-bad-type.json: In struct 'AliasStruct0':
+alias-source-elem-bad-type.json:1: member of alias member 'source' requires a string name
diff --git a/tests/qapi-schema/alias-source-elem-bad-type.json b/tests/qapi-schema/alias-source-elem-bad-type.json
new file mode 100644
index 0000000000..1d08f56492
--- /dev/null
+++ b/tests/qapi-schema/alias-source-elem-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': ['foo', true] } ] }
diff --git a/tests/qapi-schema/alias-source-elem-bad-type.out b/tests/qapi-schema/alias-source-elem-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-source-empty.err b/tests/qapi-schema/alias-source-empty.err
new file mode 100644
index 0000000000..2848e762cb
--- /dev/null
+++ b/tests/qapi-schema/alias-source-empty.err
@@ -0,0 +1,2 @@
+alias-source-empty.json: In struct 'AliasStruct0':
+alias-source-empty.json:1: alias member 'source' must not be empty
diff --git a/tests/qapi-schema/alias-source-empty.json b/tests/qapi-schema/alias-source-empty.json
new file mode 100644
index 0000000000..74b529de4a
--- /dev/null
+++ b/tests/qapi-schema/alias-source-empty.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': [] } ] }
diff --git a/tests/qapi-schema/alias-source-empty.out b/tests/qapi-schema/alias-source-empty.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alias-unknown-key.err b/tests/qapi-schema/alias-unknown-key.err
new file mode 100644
index 0000000000..c7b8cb9498
--- /dev/null
+++ b/tests/qapi-schema/alias-unknown-key.err
@@ -0,0 +1,3 @@
+alias-unknown-key.json: In struct 'AliasStruct0':
+alias-unknown-key.json:1: 'aliases' member has unknown key 'known'
+Valid keys are 'name', 'source'.
diff --git a/tests/qapi-schema/alias-unknown-key.json b/tests/qapi-schema/alias-unknown-key.json
new file mode 100644
index 0000000000..cdb8fc3d07
--- /dev/null
+++ b/tests/qapi-schema/alias-unknown-key.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': [ { 'name': 'bar', 'source': ['foo'], 'known': false } ] }
diff --git a/tests/qapi-schema/alias-unknown-key.out b/tests/qapi-schema/alias-unknown-key.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/aliases-bad-type.err b/tests/qapi-schema/aliases-bad-type.err
new file mode 100644
index 0000000000..7ffe789ec0
--- /dev/null
+++ b/tests/qapi-schema/aliases-bad-type.err
@@ -0,0 +1,2 @@
+aliases-bad-type.json: In struct 'AliasStruct0':
+aliases-bad-type.json:1: 'aliases' must be an array
diff --git a/tests/qapi-schema/aliases-bad-type.json b/tests/qapi-schema/aliases-bad-type.json
new file mode 100644
index 0000000000..4bbf6d6b20
--- /dev/null
+++ b/tests/qapi-schema/aliases-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'AliasStruct0',
+ 'data': { 'foo': 'int' },
+ 'aliases': 'this must be an array' }
diff --git a/tests/qapi-schema/aliases-bad-type.out b/tests/qapi-schema/aliases-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index 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..28cb0d34bf 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': [ { 'name': 'bar', 'source': ['foo'] } ] }
+{ 'struct': 'AliasStruct2',
+ 'data': { 'nested': 'AliasStruct1' },
+ 'aliases': [ { 'name': 'bar', 'source': ['nested', 'foo'] } ] }
+{ 'struct': 'AliasStruct3',
+ 'data': { 'nested': 'AliasStruct1' },
+ 'aliases': [ { 'source': ['nested'] } ] }
+
+{ 'union': 'AliasFlatUnion',
+ 'base': { 'tag': 'FeatureEnum1' },
+ 'discriminator': 'tag',
+ 'data': { 'eins': 'FeatureStruct1' },
+ 'aliases': [ { 'name': '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 3b1387d9f1..84f11e8702 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.29.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/6] qapi: Add interfaces for alias support to Visitor
2021-02-11 18:31 ` [PATCH v2 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
@ 2021-02-16 11:56 ` Markus Armbruster
0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-16 11:56 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 | 44 +++++++++++++++++++++++++++++++++++++
> qapi/qapi-visit-core.c | 22 +++++++++++++++++++
> 3 files changed, 78 insertions(+)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 7362c043be..d9a6874528 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 *name, 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..2ecbc20624 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
Still missing: an update of the big comment. That's okay, we can do
that last.
> @@ -459,6 +459,50 @@ void visit_end_alternate(Visitor *v, void **obj);
> */
> bool visit_optional(Visitor *v, const char *name, bool *present);
>
> +/*
> + * Defines a new alias rule.
> + *
> + * If @name is non-NULL, the member called @name in the external
> + * representation of the currently visited object is defined as an
> + * alias for the member described by @source. It is not allowed to
> + * call this function when the currently visited type is not an
> + * object.
> + *
> + * If @name is NULL, all members of the object described by @source
> + * are considered to have alias members with the same key in the
> + * currently visited object.
> + *
> + * @source is a NULL-terminated non-empty array of names that describe
> + * the path to a member, starting from the currently visited object.
> + * All elements in @source except the last one should describe
> + * objects. If an intermediate element refers to a member with a
> + * non-object type, the alias won't work (this case can legitimately
> + * happen in unions where an alias only makes sense for one branch,
> + * but not for another).
A sufficiently paranoid reader will now realize that the system won't
catch mistakes. I'm not objecting to that; I understand distinguishing
aliases that may work (just not now) from aliases that can't work is
non-trivial and quite probably not worth it. I'm just wondering whether
we should be even more explicit, to help insufficiently paranoid
readers.
Here's a possible argument against: we expect this function to be used
only by generated code, and the maintainers of the generator are
expected to be sufficiently paranoid.
The same issue might exist in qapi-code-gen.txt.
> + *
> + * 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 *name, 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..651dd88e02 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -135,6 +135,28 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
> return *present;
> }
>
> +void visit_define_alias(Visitor *v, const char *name, const char **source)
> +{
> + assert(source[0] != NULL);
> + if (v->define_alias) {
> + v->define_alias(v, name, 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;
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/6] qapi: Remember alias definitions in qobject-input-visitor
2021-02-11 18:31 ` [PATCH v2 2/6] qapi: Remember alias definitions in qobject-input-visitor Kevin Wolf
@ 2021-02-16 12:06 ` Markus Armbruster
0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-16 12:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
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 | 145 +++++++++++++++++++++++++++++++++++
> 1 file changed, 145 insertions(+)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 23843b242e..aa95cd49bd 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -29,6 +29,50 @@
> #include "qemu/cutils.h"
> #include "qemu/option.h"
>
> +/*
> + * Describes an alias that is relevant for the current StackObject,
> + * either because it aliases a member of the currently visited object
> + * or because it aliases a member of a nested object.
> + *
> + * When processing a nested object, all InputVisitorAlias objects that
> + * are relevant for the nested object are propagated, i.e. copied with
> + * the name of the nested object removed from @source.
> + */
> +typedef struct InputVisitorAlias {
> + /* StackObject in which the alias was defined */
> + struct StackObject *alias_so;
> +
> + /*
> + * Alias name as defined for @alias_so.
> + * NULL means that this is a wildcard alias, i.e. all members of
> + * @src get an alias in @alias_so with the same name.
> + */
> + const char *name;
> +
> + /*
> + * NULL terminated array representing a path to the source member
NULL-terminated
> + * that the alias refers to.
> + *
> + * Must contain at least one non-NULL element if @alias is not NULL.
> + *
> + * If it contains no non-NULL element, @alias_so must be different
> + * from the StackObject which contains this InputVisitorAlias in
> + * its aliases list. In this case, all elements in the currently
> + * visited object have an alias with the same name in @alias_so.
> + */
> + const char **src;
> +
> + /*
> + * The alias remains valid as long as the StackObject which
> + * contains this InputVisitorAlias in its aliases list 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 +82,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; /* Number of open alias scopes */
> +
> QSLIST_ENTRY(StackObject) node; /* parent */
> } StackObject;
>
> @@ -203,6 +250,43 @@ static const char *qobject_input_get_keyval(QObjectInputVisitor *qiv,
> return qstring_get_str(qstr);
> }
>
> +/*
> + * Propagate 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 @dst->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. @dst->name) removed from the source path.
> + */
> +static void propagate_aliases(StackObject *dst, StackObject *src)
> +{
> + InputVisitorAlias *a;
> +
> + QSLIST_FOREACH(a, &src->aliases, next) {
> + if (!a->src[0] || strcmp(a->src[0], dst->name)) {
> + continue;
> + }
> +
> + /*
> + * If this is not a wildcard alias, but a->src[1] is NULL,
> + * then it referred to dst->name in src and doesn't apply
> + * inside dst any more.
> + */
> + if (a->src[1] || !a->name) {
The comment explains "if COND then there is nothing to do". The code
that follows it does "if (!COND) { do stuff }". Works, but I had to
stop and re-read to get it.
How do you like
if (a->name && !a->src[1]) {
continue;
}
do stuff
?
> + InputVisitorAlias *alias = g_new(InputVisitorAlias, 1);
> +
> + *alias = (InputVisitorAlias) {
> + .name = a->name,
> + .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 +310,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));
> + }
> } else {
> assert(qlist);
> tos->entry = qlist_first(qlist);
> @@ -257,10 +344,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 +368,54 @@ 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 *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 become empty during alias propagation for
> + * wildcard aliases, but not when defining an alias (it would map
> + * all names onto themselves, which doesn't make sense).
> + */
> + assert(source[0]);
> +
> + *alias = (InputVisitorAlias) {
> + .name = 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 +838,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] 27+ messages in thread
* Re: [PATCH v2 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor
2021-02-11 18:31 ` [PATCH v2 3/6] qapi: Simplify full_name_nth() " Kevin Wolf
@ 2021-02-16 12:22 ` Markus Armbruster
0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-16 12:22 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.
Is the last sentence still accurate?
> 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.
No such case exists, but the next patch adds one. Correct?
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> qapi/qobject-input-visitor.c | 43 ++++++++++++++++++++----------------
> 1 file changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index aa95cd49bd..dd04ef0027 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -108,20 +108,20 @@ 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 a member in @so which @qiv is currently
> + * visiting. If the currently visited thing is an object, @name is
> + * the (local) name of the member to describe. If it is a list, @name
> + * is ignored and the current index (so->index) is included.
> + *
> + * If @skip_member is true, find the full name of @so itself instead.
> + * @name must be NULL then.
> + *
> + * 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,
> + bool skip_member, StackObject *so)
> {
> - StackObject *so;
> char buf[32];
>
> if (qiv->errname) {
> @@ -130,10 +130,14 @@ 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 (skip_member && so) {
> + assert(name == NULL);
> + 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 {
> @@ -144,7 +148,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);
> @@ -159,7 +162,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, false, tos);
> }
>
> static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
> @@ -503,7 +508,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, true, tos));
> return false;
> }
> return true;
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 6/6] tests/qapi-schema: Test cases for aliases
2021-02-11 18:31 ` [PATCH v2 6/6] tests/qapi-schema: Test cases " Kevin Wolf
@ 2021-02-16 15:14 ` Markus Armbruster
2021-02-16 15:31 ` Kevin Wolf
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-16 15:14 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
[...]
> diff --git a/tests/qapi-schema/alias-name-bad-type.err b/tests/qapi-schema/alias-name-bad-type.err
> new file mode 100644
> index 0000000000..489f45ff9b
> --- /dev/null
> +++ b/tests/qapi-schema/alias-name-bad-type.err
> @@ -0,0 +1,2 @@
> +alias-name-bad-type.json: In struct 'AliasStruct0':
> +alias-name-bad-type.json:1: alias member 'name' requires a string name
Would "'aliases' member 'name'..." 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..17442d5939
> --- /dev/null
> +++ b/tests/qapi-schema/alias-name-bad-type.json
> @@ -0,0 +1,3 @@
> +{ 'struct': 'AliasStruct0',
> + 'data': { 'foo': 'int' },
> + 'aliases': [ { 'name': ['bar'], 'source': ['foo'] } ] }
> diff --git a/tests/qapi-schema/alias-name-bad-type.out b/tests/qapi-schema/alias-name-bad-type.out
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/qapi-schema/alias-source-bad-type.err b/tests/qapi-schema/alias-source-bad-type.err
> new file mode 100644
> index 0000000000..b1779cbb8e
> --- /dev/null
> +++ b/tests/qapi-schema/alias-source-bad-type.err
> @@ -0,0 +1,2 @@
> +alias-source-bad-type.json: In struct 'AliasStruct0':
> +alias-source-bad-type.json:1: alias member 'source' must be an array
Would "'aliases' member 'source'..." be more consistent?
> diff --git a/tests/qapi-schema/alias-source-bad-type.json b/tests/qapi-schema/alias-source-bad-type.json
> new file mode 100644
> index 0000000000..d6a7430ee3
> --- /dev/null
> +++ b/tests/qapi-schema/alias-source-bad-type.json
> @@ -0,0 +1,3 @@
> +{ 'struct': 'AliasStruct0',
> + 'data': { 'foo': 'int' },
> + 'aliases': [ { 'name': 'bar', 'source': 'foo' } ] }
> diff --git a/tests/qapi-schema/alias-source-bad-type.out b/tests/qapi-schema/alias-source-bad-type.out
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/qapi-schema/alias-source-elem-bad-type.err b/tests/qapi-schema/alias-source-elem-bad-type.err
> new file mode 100644
> index 0000000000..f73fbece77
> --- /dev/null
> +++ b/tests/qapi-schema/alias-source-elem-bad-type.err
> @@ -0,0 +1,2 @@
> +alias-source-elem-bad-type.json: In struct 'AliasStruct0':
> +alias-source-elem-bad-type.json:1: member of alias member 'source' requires a string name
Would "member of 'aliases' member 'source'..." be more consistent?
[...]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 6/6] tests/qapi-schema: Test cases for aliases
2021-02-16 15:14 ` Markus Armbruster
@ 2021-02-16 15:31 ` Kevin Wolf
2021-02-16 16:14 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2021-02-16 15:31 UTC (permalink / raw)
To: Markus Armbruster; +Cc: jsnow, qemu-devel
Am 16.02.2021 um 16:14 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> [...]
> > diff --git a/tests/qapi-schema/alias-name-bad-type.err b/tests/qapi-schema/alias-name-bad-type.err
> > new file mode 100644
> > index 0000000000..489f45ff9b
> > --- /dev/null
> > +++ b/tests/qapi-schema/alias-name-bad-type.err
> > @@ -0,0 +1,2 @@
> > +alias-name-bad-type.json: In struct 'AliasStruct0':
> > +alias-name-bad-type.json:1: alias member 'name' requires a string name
>
> Would "'aliases' member 'name'..." be more consistent?
'aliases' is a list, not a single alias definition, so technically it
would have to be "'aliases' member member 'name'...", which I feel is a
bit too confusing.
I think I have consistently used "alias" for "'aliases' member"
everywhere, though. At least, that was the intention.
Kevin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/6] qapi: Add support for aliases
2021-02-11 18:31 ` [PATCH v2 5/6] qapi: Add support for aliases Kevin Wolf
@ 2021-02-16 15:43 ` Markus Armbruster
2021-02-17 15:23 ` Markus Armbruster
1 sibling, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-16 15:43 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 | 105 ++++++++++++++++++++++++-
> docs/sphinx/qapidoc.py | 2 +-
> scripts/qapi/expr.py | 36 ++++++++-
> scripts/qapi/schema.py | 30 +++++--
> scripts/qapi/types.py | 4 +-
> scripts/qapi/visit.py | 34 +++++++-
> 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, 203 insertions(+), 19 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 6906a06ad2..247c4b8ef4 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,
> @@ -279,6 +280,9 @@ 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.
> +
>
> === Union types ===
>
> @@ -286,13 +290,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 }
> @@ -402,6 +408,9 @@ 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.
> +
>
> === Alternate types ===
>
> @@ -837,6 +846,96 @@ 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.
I get what you mean by "external representation", but let's use "wire"
for consistency with existing text. It's defined in section
"Introduction", and used throughout the text.
Aside: we'll eventually have to revamp the document to cater for "client
protocols" other than JSON, such as the one we get with keyval_parse()
and qobject_input_visitor_new_keyval().
Moreover, aliases apply just to the input direction of the wire.
What about "may be used in wire input"?
> +
> +Syntax:
> + ALIASES = [ ALIAS, ... ]
> + ALIAS = { '*name': STRING,
> + 'source': [ STRING, ... ] }
> +
> +If 'name' is present, then the single member referred to by 'source'
> +is made accessible with the name given in 'name' in the type where the
"given by 'name'"?
> +alias definition is specified.
> +
> +If 'name' 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' is a non-empty list of member names representing the path to
> +an object member. The first name is resolved in the same object. Each
> +subsequent member is resolved in the object named by the preceding
> +member.
> +
> +Example: Alternative name for a member in the same object (the member
> +"path" may be given through its alias "filename" in the external
> +representation):
> +
> +{ 'struct': 'File',
> + 'data': { 'path': 'str' },
> + 'aliases': [ { 'name': 'filename', 'source': ['path'] } ] }
What about
Example: Alternative name for a member in the same object
{ 'struct': 'File',
'data': { 'path': 'str' },
'aliases': [ { 'name': 'filename', 'source': ['path'] } ] }
Member "path" may instead be given through its alias "filename" in
input.
> +
> +Example: Alias for a member in a nested object:
Let's drop the second colon.
> +
> +{ 'struct': 'A',
> + 'data': { 'zahl': 'int' } }
> +{ 'struct': 'B',
> + 'data': { 'drei': 'A' } }
> +{ 'struct': 'C',
> + 'data': { 'zwei': 'B' } }
> +{ 'struct': 'D',
> + 'data': { 'eins': 'C' },
> + 'aliases': [ { 'name': 'number',
> + 'source': ['eins', 'zwei', 'drei', 'zahl' ] },
> + { 'name': 'the_B',
> + 'source': ['eins','zwei'] } ] }
> +
> +With this definition, each of the following inputs mean the same:
"inputs for 'D'"
> +
> +* { 'eins': { 'zwei': { 'drei': { 'zahl': 42 } } } }
> +
> +* { 'the_B': { 'drei': { 'zahl': 42 } } }
> +
> +* { 'number': 42 }
> +
> +Example: Flattening a union with a wildcard alias that maps all
> +members of 'data' to the top level:
"Flattening a simple union"
Let's drop the second colon.
> +
> +{ 'union': 'SocketAddress',
> + 'data': {
> + 'inet': 'InetSocketAddress',
> + 'unix': 'UnixSocketAddress' },
> + 'aliases': [ { 'source': ['data'] } ] }
> +
> +Aliases are transitive: 'source' may refer to another alias name. In
> +this case, the alias is effectively an altenative name for the source
Typo in "alternative".
> +of the other alias.
> +
> +Example: Giving "the_answer" on the top level provides a value for
> +"zahl" in the nested object:
> +
> +{ 'struct': 'A',
> + 'data': { 'zahl': 'int' },
> + 'aliases': [ { 'name': 'number', 'source': ['zahl'] } ] }
> +{ 'struct': 'B',
> + 'data': { 'nested': 'A' },
> + 'aliases': [ { 'name': 'the_answer',
> + 'source': ['nested', 'number'] } ] }
Do we need both this one and the "Example: Alias for a member in a
nested object" above?
> +
> +In order to accommodate unions where variants differ in structure, 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. Note that the QAPI generator does not check whether
> +there is at least one branch for which an alias could match. If a
> +source member is misspelt, the alias just won't work.
In my review of PATCH 1, I wondered whether this patch would be
sufficiently explicit on "does not check". It is.
> +
> +
> === 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..743e23ec85 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -198,6 +198,34 @@ 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' members must be objects")
> + check_keys(a, info, "'aliases' member", ['source'], ['name'])
> +
> + if 'name' in a:
> + source = "alias member 'name'"
> + check_name_is_str(a['name'], info, source)
> + check_name_str(a['name'], info, source)
> +
> + if not isinstance(a['source'], list):
> + raise QAPISemError(info,
> + "alias member 'source' must be an array")
> + if not a['source']:
> + raise QAPISemError(info,
> + "alias member 'source' must not be empty")
> +
> + source = "member 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 +256,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 +274,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 +362,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 +373,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 353e8020a2..14a2b0175b 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -118,7 +118,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,
> @@ -362,9 +362,19 @@ class QAPISchemaArrayType(QAPISchemaType):
> return "%s type ['%s']" % (self.meta, self._element_type_name)
>
>
> +class QAPISchemaAlias:
> + def __init__(self, name, source):
> + assert name is None or isinstance(name, str)
> + assert source
> + for member in source:
> + assert isinstance(member, str)
> + self.name = name
> + 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
> @@ -382,6 +392,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
> @@ -474,7 +485,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)
> @@ -964,6 +975,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('name'), 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]
> @@ -1038,11 +1055,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)
> @@ -1061,6 +1079,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
> @@ -1085,7 +1104,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 2bdd626847..c8306479f5 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 22e62df901..e370485f6e 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,24 @@ 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.name:
> + name = '"%s"' % a.name
> + else:
> + name = "NULL"
> +
> + source = ", ".join('"%s"' % x for x in a.source)
> +
> + ret += mcgen('''
> + visit_define_alias(v, %(name)s, (const char * []) { %(source)s, NULL });
The more old-fashioned
visit_define_alias(v, "bar", "nested", "foo", NULL);
with variadic visit_define_alias() feels tempting.
> +''',
> + name=name, source=source)
> +
> if base:
> ret += mcgen('''
> if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
> @@ -133,6 +153,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 +386,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))
> + 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..1679d1b5da 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.name:
> + print(' alias %s -> %s' % (a.name, '.'.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'.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 6/6] tests/qapi-schema: Test cases for aliases
2021-02-16 15:31 ` Kevin Wolf
@ 2021-02-16 16:14 ` Markus Armbruster
2021-02-17 12:23 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-16 16:14 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 16.02.2021 um 16:14 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> [...]
>> > diff --git a/tests/qapi-schema/alias-name-bad-type.err b/tests/qapi-schema/alias-name-bad-type.err
>> > new file mode 100644
>> > index 0000000000..489f45ff9b
>> > --- /dev/null
>> > +++ b/tests/qapi-schema/alias-name-bad-type.err
>> > @@ -0,0 +1,2 @@
>> > +alias-name-bad-type.json: In struct 'AliasStruct0':
>> > +alias-name-bad-type.json:1: alias member 'name' requires a string name
>>
>> Would "'aliases' member 'name'..." be more consistent?
>
> 'aliases' is a list, not a single alias definition, so technically it
> would have to be "'aliases' member member 'name'...", which I feel is a
> bit too confusing.
Indeed.
I think glossing over the list is excusable.
> I think I have consistently used "alias" for "'aliases' member"
> everywhere, though. At least, that was the intention.
A different way of glossing over details. Should do as well. I'll
double-check consistency.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 6/6] tests/qapi-schema: Test cases for aliases
2021-02-16 16:14 ` Markus Armbruster
@ 2021-02-17 12:23 ` Markus Armbruster
0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-17 12:23 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Markus Armbruster <armbru@redhat.com> writes:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 16.02.2021 um 16:14 hat Markus Armbruster geschrieben:
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>>
>>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> [...]
>>> > diff --git a/tests/qapi-schema/alias-name-bad-type.err b/tests/qapi-schema/alias-name-bad-type.err
>>> > new file mode 100644
>>> > index 0000000000..489f45ff9b
>>> > --- /dev/null
>>> > +++ b/tests/qapi-schema/alias-name-bad-type.err
>>> > @@ -0,0 +1,2 @@
>>> > +alias-name-bad-type.json: In struct 'AliasStruct0':
>>> > +alias-name-bad-type.json:1: alias member 'name' requires a string name
>>>
>>> Would "'aliases' member 'name'..." be more consistent?
>>
>> 'aliases' is a list, not a single alias definition, so technically it
>> would have to be "'aliases' member member 'name'...", which I feel is a
>> bit too confusing.
>
> Indeed.
>
> I think glossing over the list is excusable.
>
>> I think I have consistently used "alias" for "'aliases' member"
>> everywhere, though. At least, that was the intention.
>
> A different way of glossing over details. Should do as well. I'll
> double-check consistency.
I did, and it looks okay:
$ grep "'alias" *err
alias-bad-type.err:alias-bad-type.json:1: 'aliases' members must be objects
Okay; we are talking about members of array 'aliases' here.
alias-missing-source.err:alias-missing-source.json:1: 'aliases' member misses key 'source'
Likewise.
alias-unknown-key.err:alias-unknown-key.json:1: 'aliases' member has unknown key 'known'
Likewise.
aliases-bad-type.err:aliases-bad-type.json:1: 'aliases' must be an array
Okay; we are talking about 'aliases'.
double-type.err:Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'.
unknown-expr-key.err:Valid keys are 'aliases', 'base', 'data', 'features', 'if', 'struct'.
Okay.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/6] qapi: Add support for aliases
2021-02-11 18:31 ` [PATCH v2 5/6] qapi: Add support for aliases Kevin Wolf
2021-02-16 15:43 ` Markus Armbruster
@ 2021-02-17 15:23 ` Markus Armbruster
2021-02-17 16:17 ` Kevin Wolf
1 sibling, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-17 15:23 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>
[...]
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 22e62df901..e370485f6e 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,24 @@ 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.name:
> + name = '"%s"' % a.name
> + else:
> + name = "NULL"
> +
> + source = ", ".join('"%s"' % x for x in a.source)
> +
> + ret += mcgen('''
> + visit_define_alias(v, %(name)s, (const char * []) { %(source)s, NULL });
> +''',
> + name=name, source=source)
> +
> if base:
> ret += mcgen('''
> if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
> @@ -133,6 +153,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;
> }
The visit_type_FOO_members() are primarily helpers for the
visit_type_FOO(). But they also get called
* by visit_type_BAR_members() when
- struct or union BAR has 'base': 'FOO'
- union or alternate BAR a variant 'FOO'
* by qmp_marshal_CMD() when
- CMD has 'boxed': true, 'data': 'FOO'
Have you considered these cases?
How's the test coverage?
> @@ -361,14 +386,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))
> + 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_'):
[...]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
2021-02-11 18:31 ` [PATCH v2 4/6] qapi: Apply aliases " Kevin Wolf
@ 2021-02-17 15:32 ` Markus Armbruster
2021-02-17 17:50 ` Kevin Wolf
2021-02-19 14:42 ` Markus Armbruster
2021-02-24 8:28 ` Markus Armbruster
2 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-17 15:32 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>
Looking just at qobject_input_try_get_object() for now.
> ---
> qapi/qobject-input-visitor.c | 214 +++++++++++++++++++++++++++++++++--
> 1 file changed, 205 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index dd04ef0027..3ea5e5abd6 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -95,6 +95,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;
> @@ -167,9 +169,178 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> return full_name_so(qiv, name, false, tos);
> }
>
> +static bool find_object_member(QObjectInputVisitor *qiv,
> + StackObject **so, const char **name,
> + bool *implicit_object, Error **errp);
> +
> +/*
> + * Check whether the alias member defined by @a is present in the
> + * input and can be used to obtain the value for the member @name in
> + * the currently visited object.
> + */
> +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->name) {
> + name = a->name;
> + }
> +
> + /*
> + * Check whether the alias member is present in the input
> + * (possibly recursively because aliases are transitive).
> + */
> + 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;
> +}
> +
> +/*
> + * Check whether the member @name in the object visited by @so can be
> + * specified in the input by using the alias described by @a.
> + *
> + * If @name is only a prefix of the alias source, but doesn't match
> + * immediately, false is returned and @implicit_object is set to true
> + * if it is non-NULL. In all other cases, @implicit_object is left
> + * unchanged.
> + */
> +static bool alias_source_matches(QObjectInputVisitor *qiv,
> + StackObject *so, InputVisitorAlias *a,
> + const char *name, bool *implicit_object)
> +{
> + if (a->src[0] == NULL) {
> + assert(a->name == NULL);
> + return true;
> + }
> +
> + if (!strcmp(a->src[0], name)) {
> + if (a->name && 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->name || alias_present(qiv, a, a->name)) {
> + *implicit_object = true;
> + }
> + }
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Find the place in the input where the value for the object member
> + * @name in @so is specified, considering applicable aliases.
> + *
> + * If a value could be found, true is returned and @so and @name are
> + * updated to identify the key name and StackObject where the value
> + * can be found in the input. (This is either unchanged or the
> + * alias_so/name of an alias.) The value of @implicit_object on
> + * return is undefined in this case.
> + *
> + * If no value could be found in the input, false is returned. This
> + * is not an error and @errp remains unchanged. If @implicit_object
> + * is non-NULL, it is set to true if the given name is a prefix of the
> + * source path of an alias for which a value may be present in the
> + * input. It is set to false otherwise.
> + *
> + * If an error occurs (e.g. two values are specified for the member
> + * through different names), false is returned and @errp is set. The
> + * value of @implicit_object on return is undefined in this case.
> + */
> +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->name from a->alias_so.
> + */
> + QSLIST_FOREACH(a, &cur_so->aliases, next) {
> + if (a->name == 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, false, *so));
> + return false;
> + } else {
> + found = a->name ?: *name;
> + *so = a->alias_so;
> + found_is_wildcard = !a->name;
> + }
> + }
> +
> + /* 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)
Reminder:
* 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.
> {
> StackObject *tos;
> QObject *qobj;
QObject *ret;
if (QSLIST_EMPTY(&qiv->stack)) {
/* Starting at root, name is ignored. */
assert(qiv->root);
return qiv->root;
}
/* We are in a container; find the next element. */
tos = QSLIST_FIRST(&qiv->stack);
qobj = tos->obj;
> @@ -187,10 +358,30 @@ static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
> assert(qobj);
>
> if (qobject_type(qobj) == QTYPE_QDICT) {
We're visiting a member of object @qobj, which is tos->obj.
> - 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)) {
No input found.
> + if (implicit_object) {
> + /*
> + * The member is not present in the input, but
> + * something inside of it might still be given through
> + * an alias. Pretend there was an empty object in the
> + * input.
> + */
Cue me scratching head.
find_object_member()'s contract explains @implicit_object is true "if
the given name is a prefix of the source path of an alias for which a
value may be present in the input."
I figure this means qiv->stack has a stack object with an alias whose
src[] has a prefix that resolves to @tos (the currently visited object).
Visiting the members of the currently visited object may or may not
visit something where that src[] resolves completely, and that something
may or may not have a member for the alias.
If it does, there is input, and "something" will happen to route it to
the correct place. What exactly? Should become clearer further down.
If it doesn't, there is no input.
> + if (!qiv->empty_qdict) {
> + qiv->empty_qdict = qdict_new();
> + }
> + return QOBJECT(qiv->empty_qdict);
So far, I have no idea *why* we pretend there was an empty object in the
input, why sharing it is safe, and why changing the return value from
null to empty dict is okay. Should become clearer further down.
> + } else {
> + return NULL;
There is definitely no input.
The old code returns null then (because the qdict_get() above returns
null). No change. Good.
> + }
> + }
We get here only when find_object_member() found input.
If no aliases applied, @so and @key are unchanged, i.e. @so is @tos, and
@key is @name. The code below is the old code with @qobj replaced by
so->obj and @name replaced by @key. No change. Good.
Else, some alias (or chain of aliases) applied.
Let's assume a single alias for now. It is defined for a stack object
further down qiv->stack, with an alias name there, and it's src[]
resolves to @tos (the currently visited object).
find_object_member() found input there, i.e. the alias stack object's
input has a member with the alias name. It changed @so to the alias
stack object, and @key to the alias name. The code below then gets the
input value from member with the alias name in the alias object.
This consumes the "alias member" of the outer input object, and uses it
as input for the "true member" of the inner object.
Good.
I figure a chain of aliases works the same; the only difference is we
need more steps to find the alias stack object and name.
Correct?
> + ret = qdict_get(qobject_to(QDict, so->obj), key);
Any particular reason why find_object_member() doesn't simply return the
input it found?
> + if (so->h && consume && ret) {
How can @ret be null?
> + bool removed = g_hash_table_remove(so->h, key);
> assert(removed);
> }
> } else {
Note that we access the parent container via tos->obj, so->obj, and
qobj. Too many aliases for my taste. Let's eliminate qobj.
Now let me try to figure out how the magic around @implicit_object
works.
Okay, I came, I saw, I ran out of brain juice. Help!
Please walk me through an example that covers all the paths through this
function, unless you have better ideas on how to explain it.
> @@ -216,9 +407,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;
> @@ -799,13 +991,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;
> }
>
> @@ -820,6 +1015,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] 27+ messages in thread
* Re: [PATCH v2 5/6] qapi: Add support for aliases
2021-02-17 15:23 ` Markus Armbruster
@ 2021-02-17 16:17 ` Kevin Wolf
2021-02-18 10:26 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2021-02-17 16:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: jsnow, qemu-devel
Am 17.02.2021 um 16:23 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>
> [...]
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 22e62df901..e370485f6e 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,24 @@ 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.name:
> > + name = '"%s"' % a.name
> > + else:
> > + name = "NULL"
> > +
> > + source = ", ".join('"%s"' % x for x in a.source)
> > +
> > + ret += mcgen('''
> > + visit_define_alias(v, %(name)s, (const char * []) { %(source)s, NULL });
> > +''',
> > + name=name, source=source)
> > +
> > if base:
> > ret += mcgen('''
> > if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
> > @@ -133,6 +153,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;
> > }
>
> The visit_type_FOO_members() are primarily helpers for the
> visit_type_FOO(). But they also get called
>
> * by visit_type_BAR_members() when
>
> - struct or union BAR has 'base': 'FOO'
> - union or alternate BAR a variant 'FOO'
>
> * by qmp_marshal_CMD() when
>
> - CMD has 'boxed': true, 'data': 'FOO'
>
> Have you considered these cases?
>
> How's the test coverage?
What is the difference between these cases? The visiting should work the
same, no matter from where it was started.
I did consider the struct base/union variant case and this is why I
introduced visit_start/end_alias_scope so that aliases wouldn't leak to
the outer level.
Now that I'm trying to think of a test case, this probably only protects
against weird corner cases: The input object is the same anyway, so I
guess the only way for this to make a difference is when the base struct
defines an alias for a member that it doesn't even have (so the alias
would remain unused when applied to the base struct independently), but
that exists in the struct in which it is embedded.
I hope adding a test case that checks that this is an error should be
easy. Would the right place be tests/test-qobject-input-visitor.c?
Can you think of any other specific differences that need to be tested?
Kevin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
2021-02-17 15:32 ` Markus Armbruster
@ 2021-02-17 17:50 ` Kevin Wolf
2021-02-18 13:39 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2021-02-17 17:50 UTC (permalink / raw)
To: Markus Armbruster; +Cc: jsnow, qemu-devel
Am 17.02.2021 um 16:32 hat Markus Armbruster geschrieben:
> 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>
>
> Looking just at qobject_input_try_get_object() for now.
:-(
This patch doesn't even feel that complicated to me.
Old: Get the value from the QDict of the current StackObject with the
given name.
New: First do alias resolution (with find_object_member), which results
in a StackObject and a name, and that's the QDict and key where you get
the value from.
Minor complication: Aliases can refer to members of nested objects that
may not be provided in the input. But we want these to work.
For example, my chardev series defines aliases to flatten
SocketAddressLegacy (old syntax, I haven't rebased it yet):
{ 'union': 'SocketAddressLegacy',
'data': {
'inet': 'InetSocketAddress',
'unix': 'UnixSocketAddress',
'vsock': 'VsockSocketAddress',
'fd': 'String' },
'aliases': [
{'source': ['data']},
{'alias': 'fd', 'source': ['data', 'str']}
]}
Of course, the idea is that this input should work:
{ 'type': 'inet', 'hostname': 'localhost', 'port': '1234' }
However, without implicit objects, parsing 'data' fails because it
expects an object, but none is given (we specified all of its members on
the top level through aliases). What we would have to give is:
{ 'type': 'inet', 'hostname': 'localhost', 'port': '1234', 'data': {} }
And _that_ would work. Visiting 'data' succeeds because we now have the
object that the visitor expects, and when visiting its members, the
aliases fill in all of the mandatory values.
So what this patch does is to implicitly assume the 'data': {} instead
of erroring out when we know that aliases exist that can still provide
values for the content of 'data'.
> > ---
> > qapi/qobject-input-visitor.c | 214 +++++++++++++++++++++++++++++++++--
> > 1 file changed, 205 insertions(+), 9 deletions(-)
> >
> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> > index dd04ef0027..3ea5e5abd6 100644
> > --- a/qapi/qobject-input-visitor.c
> > +++ b/qapi/qobject-input-visitor.c
> > @@ -95,6 +95,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;
> > @@ -167,9 +169,178 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> > return full_name_so(qiv, name, false, tos);
> > }
> >
> > +static bool find_object_member(QObjectInputVisitor *qiv,
> > + StackObject **so, const char **name,
> > + bool *implicit_object, Error **errp);
> > +
> > +/*
> > + * Check whether the alias member defined by @a is present in the
> > + * input and can be used to obtain the value for the member @name in
> > + * the currently visited object.
> > + */
> > +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->name) {
> > + name = a->name;
> > + }
> > +
> > + /*
> > + * Check whether the alias member is present in the input
> > + * (possibly recursively because aliases are transitive).
> > + */
> > + 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;
> > +}
> > +
> > +/*
> > + * Check whether the member @name in the object visited by @so can be
> > + * specified in the input by using the alias described by @a.
> > + *
> > + * If @name is only a prefix of the alias source, but doesn't match
> > + * immediately, false is returned and @implicit_object is set to true
> > + * if it is non-NULL. In all other cases, @implicit_object is left
> > + * unchanged.
> > + */
> > +static bool alias_source_matches(QObjectInputVisitor *qiv,
> > + StackObject *so, InputVisitorAlias *a,
> > + const char *name, bool *implicit_object)
> > +{
> > + if (a->src[0] == NULL) {
> > + assert(a->name == NULL);
> > + return true;
> > + }
> > +
> > + if (!strcmp(a->src[0], name)) {
> > + if (a->name && 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->name || alias_present(qiv, a, a->name)) {
> > + *implicit_object = true;
> > + }
> > + }
> > + }
> > +
> > + return false;
> > +}
> > +
> > +/*
> > + * Find the place in the input where the value for the object member
> > + * @name in @so is specified, considering applicable aliases.
> > + *
> > + * If a value could be found, true is returned and @so and @name are
> > + * updated to identify the key name and StackObject where the value
> > + * can be found in the input. (This is either unchanged or the
> > + * alias_so/name of an alias.) The value of @implicit_object on
> > + * return is undefined in this case.
> > + *
> > + * If no value could be found in the input, false is returned. This
> > + * is not an error and @errp remains unchanged. If @implicit_object
> > + * is non-NULL, it is set to true if the given name is a prefix of the
> > + * source path of an alias for which a value may be present in the
> > + * input. It is set to false otherwise.
> > + *
> > + * If an error occurs (e.g. two values are specified for the member
> > + * through different names), false is returned and @errp is set. The
> > + * value of @implicit_object on return is undefined in this case.
> > + */
> > +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->name from a->alias_so.
> > + */
> > + QSLIST_FOREACH(a, &cur_so->aliases, next) {
> > + if (a->name == 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, false, *so));
> > + return false;
> > + } else {
> > + found = a->name ?: *name;
> > + *so = a->alias_so;
> > + found_is_wildcard = !a->name;
> > + }
> > + }
> > +
> > + /* 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)
>
> Reminder:
>
> * 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.
>
> > {
> > StackObject *tos;
> > QObject *qobj;
> QObject *ret;
>
> if (QSLIST_EMPTY(&qiv->stack)) {
> /* Starting at root, name is ignored. */
> assert(qiv->root);
> return qiv->root;
> }
>
> /* We are in a container; find the next element. */
> tos = QSLIST_FIRST(&qiv->stack);
> qobj = tos->obj;
> > @@ -187,10 +358,30 @@ static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
> > assert(qobj);
> >
> > if (qobject_type(qobj) == QTYPE_QDICT) {
>
> We're visiting a member of object @qobj, which is tos->obj.
>
> > - 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)) {
>
> No input found.
>
> > + if (implicit_object) {
> > + /*
> > + * The member is not present in the input, but
> > + * something inside of it might still be given through
> > + * an alias. Pretend there was an empty object in the
> > + * input.
> > + */
>
> Cue me scratching head.
I hope the above made it clearer.
> find_object_member()'s contract explains @implicit_object is true "if
> the given name is a prefix of the source path of an alias for which a
> value may be present in the input."
>
> I figure this means qiv->stack has a stack object with an alias whose
> src[] has a prefix that resolves to @tos (the currently visited object).
No. tos->aliases contains an alias where @name is a prefix of
alias->src[] (but doesn't fully match src[]).
Or in other words, since @name is only a single element, being a prefix
means alias->src[0] == name.
The second condition ("for which a value may be present in the input")
means that if we know that the alias isn't used in the input, then we
don't create an implicit object, but use the traditinoal "not found"
path.
> Visiting the members of the currently visited object may or may not
> visit something where that src[] resolves completely, and that something
> may or may not have a member for the alias.
>
> If it does, there is input, and "something" will happen to route it to
> the correct place. What exactly? Should become clearer further down.
>
> If it doesn't, there is no input.
Visiting the aliased member will call qobject_input_try_get_object()
again, and this is where the alias will be used. We're only making sure
that it is even visited instead of erroring out earlier.
> > + if (!qiv->empty_qdict) {
> > + qiv->empty_qdict = qdict_new();
> > + }
> > + return QOBJECT(qiv->empty_qdict);
>
> So far, I have no idea *why* we pretend there was an empty object in the
> input, why sharing it is safe, and why changing the return value from
> null to empty dict is okay. Should become clearer further down.
>
> > + } else {
> > + return NULL;
>
> There is definitely no input.
>
> The old code returns null then (because the qdict_get() above returns
> null). No change. Good.
>
> > + }
> > + }
>
> We get here only when find_object_member() found input.
Yes. The interesting part of the patch ends here. We know the
StackObject and name where we find the right value.
> If no aliases applied, @so and @key are unchanged, i.e. @so is @tos, and
> @key is @name. The code below is the old code with @qobj replaced by
> so->obj and @name replaced by @key. No change. Good.
>
> Else, some alias (or chain of aliases) applied.
>
> Let's assume a single alias for now. It is defined for a stack object
> further down qiv->stack, with an alias name there, and it's src[]
> resolves to @tos (the currently visited object).
Specifically, to the member @name inside of @tos.
> find_object_member() found input there, i.e. the alias stack object's
> input has a member with the alias name. It changed @so to the alias
> stack object, and @key to the alias name. The code below then gets the
> input value from member with the alias name in the alias object.
>
> This consumes the "alias member" of the outer input object, and uses it
> as input for the "true member" of the inner object.
>
> Good.
>
> I figure a chain of aliases works the same; the only difference is we
> need more steps to find the alias stack object and name.
>
> Correct?
Yes, chains are handled inside find_object_member(), so if necessary, it
will recursively resolve aliases.
> > + ret = qdict_get(qobject_to(QDict, so->obj), key);
>
> Any particular reason why find_object_member() doesn't simply return the
> input it found?
I think it's more managable to have a function that is only responsible
for resolving names, and another to actually fetch the values from
there.
If it did both at once, qobject_input_try_get_object() wouldn't even
have anything to do any more. Getting the value from the input was its
job before the patch, so I don't see why it should be different
afterwards.
> > + if (so->h && consume && ret) {
>
> How can @ret be null?
I don't think it can because find_object_member() would return false.
This is old code that made sense when calling qdict_get() without
checking first if the value is present in the input.
> > + bool removed = g_hash_table_remove(so->h, key);
> > assert(removed);
> > }
> > } else {
>
> Note that we access the parent container via tos->obj, so->obj, and
> qobj. Too many aliases for my taste. Let's eliminate qobj.
tos->obj and so->obj are different! They can be the same (if no alias
was used or the alias source was in the same object, i.e. a simple
rename), but they don't have to.
qobj exists before this patch and isn't touched at all. I can add
another patch to remove it, but it has nothing to do with this one.
> Now let me try to figure out how the magic around @implicit_object
> works.
>
> Okay, I came, I saw, I ran out of brain juice. Help!
>
> Please walk me through an example that covers all the paths through
> this function, unless you have better ideas on how to explain it.
It is exactly one path that was added, the implicit object. The rest are
existing paths. I hope the example above clarified this path. If not,
then I'm not sure how to explain.
I can understand how the reason for having implicit objects may be
unclear (though the comments try to describe that - maybe you didn't
read it because you read my story backwards). But ignoring the why, the
how feels really obvious to me. The code that you've looked at so far
hasn't even changed much.
Kevin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/6] qapi: Add support for aliases
2021-02-17 16:17 ` Kevin Wolf
@ 2021-02-18 10:26 ` Markus Armbruster
0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-18 10:26 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 17.02.2021 um 16:23 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>
>> [...]
>> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>> > index 22e62df901..e370485f6e 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,24 @@ 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.name:
>> > + name = '"%s"' % a.name
>> > + else:
>> > + name = "NULL"
>> > +
>> > + source = ", ".join('"%s"' % x for x in a.source)
>> > +
>> > + ret += mcgen('''
>> > + visit_define_alias(v, %(name)s, (const char * []) { %(source)s, NULL });
>> > +''',
>> > + name=name, source=source)
>> > +
>> > if base:
>> > ret += mcgen('''
>> > if (!visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, errp)) {
>> > @@ -133,6 +153,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;
>> > }
>>
>> The visit_type_FOO_members() are primarily helpers for the
>> visit_type_FOO(). But they also get called
>>
>> * by visit_type_BAR_members() when
>>
>> - struct or union BAR has 'base': 'FOO'
>> - union or alternate BAR a variant 'FOO'
>>
>> * by qmp_marshal_CMD() when
>>
>> - CMD has 'boxed': true, 'data': 'FOO'
>>
>> Have you considered these cases?
>>
>> How's the test coverage?
>
> What is the difference between these cases? The visiting should work the
> same, no matter from where it was started.
>
> I did consider the struct base/union variant case and this is why I
> introduced visit_start/end_alias_scope so that aliases wouldn't leak to
> the outer level.
Good!
The qmp_marshal_CMD()'s code around visit_type_FOO_members() is fairly
close to visit_type_FOO()'s code: it avoids allocation and drops cases
that can't happen with the visitor at hand. Can't see why aliases would
not work.
"Can't see why it would not work" is of course a sensible precondition
for testing, not an excuse for not testing.
> Now that I'm trying to think of a test case, this probably only protects
> against weird corner cases: The input object is the same anyway, so I
> guess the only way for this to make a difference is when the base struct
> defines an alias for a member that it doesn't even have (so the alias
> would remain unused when applied to the base struct independently), but
> that exists in the struct in which it is embedded.
It's best to strangle such weird corner cases in the crib, before they
can conspire with other weird corner cases to make a real mess.
> I hope adding a test case that checks that this is an error should be
> easy. Would the right place be tests/test-qobject-input-visitor.c?
Sounds good to me.
> Can you think of any other specific differences that need to be tested?
I think you nailed the negative tests.
The positive tests are limited to frontend tests now, i.e. put some
valid use into qapi-schema-test.json, then check the resulting
qapi-schema-test.out looks sane. Covered there, I think:
* empty 'aliases' have no effect: AliasStruct0
Aside: we could choose to outlaw instead.
* alias a struct member: AliasStruct1
* alias a member of a struct member: AliasStruct2
* alias all members of a struct member: AliasStruct3
* alias a member of a flat union variant: AliasFlatUnion
* alias all members of all simple union variants: AliasSimpleUnion
Missing, I think:
* alias a member of the base struct
* alias a member of a member of a struct member
I figure both are relatively uninteresting as front-end tests, but unit
tests (below) will need them.
Possibly:
* alias a member of a simple union variant
* alias all members of a simple union variant
Not sure how much these buy us over the other tests. Perhaps we should
spend our limited time on killing simple unions instead (I hope aliases
will help us get there).
Aside: we could add aliases to alternates, for rewriting a non-object
variant into an object variant. Not now, and later only if we find
sufficiently compelling uses.
Unit tests go further than frontend tests: they test the generated
code. And the hand-written code, of course.
We should cover the various paths through
qobject_input_try_get_object().
I'd like us to cover its use in the various contexts, i.e. from all
kinds of callers of visit_type_BAR_members(). See above for a list.
I don't think we need to do the full test matrix. The inner workings of
qobject_input_try_get_object() look independent enough from the contexts
to justify risking a short cut. So, cover the paths, and also cover the
contexts, but don't cover the paths for each context.
What do you think?
Feel free to suggest more tests, or more shortcuts.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
2021-02-17 17:50 ` Kevin Wolf
@ 2021-02-18 13:39 ` Markus Armbruster
2021-02-18 16:10 ` Kevin Wolf
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-18 13:39 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 17.02.2021 um 16:32 hat Markus Armbruster geschrieben:
>> 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>
>>
>> Looking just at qobject_input_try_get_object() for now.
>
> :-(
>
> This patch doesn't even feel that complicated to me.
I suspect it's just me having an unusually obtuse week.
The code is straightforward enough. What I'm missing is a bit of "how
does this accomplish the goal" and "why is this safe" here and there.
> Old: Get the value from the QDict of the current StackObject with the
> given name.
>
> New: First do alias resolution (with find_object_member), which results
> in a StackObject and a name, and that's the QDict and key where you get
> the value from.
This part I understand.
We simultaneously walk the QAPI type and the input QObject, consuming
input as we go.
Whenever the walk leaves a QAPI object type, we check the corresponding
QObject has been consumed completely.
With aliases, we additionally look for input in a certain enclosing
input object (i.e. up the recursion stack). If found, consume.
> Minor complication: Aliases can refer to members of nested objects that
> may not be provided in the input. But we want these to work.
>
> For example, my chardev series defines aliases to flatten
> SocketAddressLegacy (old syntax, I haven't rebased it yet):
>
> { 'union': 'SocketAddressLegacy',
> 'data': {
> 'inet': 'InetSocketAddress',
> 'unix': 'UnixSocketAddress',
> 'vsock': 'VsockSocketAddress',
> 'fd': 'String' },
> 'aliases': [
> {'source': ['data']},
> {'alias': 'fd', 'source': ['data', 'str']}
> ]}
>
> Of course, the idea is that this input should work:
>
> { 'type': 'inet', 'hostname': 'localhost', 'port': '1234' }
>
> However, without implicit objects, parsing 'data' fails because it
> expects an object, but none is given (we specified all of its members on
> the top level through aliases). What we would have to give is:
>
> { 'type': 'inet', 'hostname': 'localhost', 'port': '1234', 'data': {} }
>
> And _that_ would work. Visiting 'data' succeeds because we now have the
> object that the visitor expects, and when visiting its members, the
> aliases fill in all of the mandatory values.
>
> So what this patch does is to implicitly assume the 'data': {} instead
> of erroring out when we know that aliases exist that can still provide
> values for the content of 'data'.
Aliases exist than can still provide, but will they? What if input is
{"type": "inet"}
?
Your explanation makes me guess this is equivalent to
{"type": "inet", "data": {}}
which fails the visit, because mandatory members of "data" are missing.
Fine.
If we make the members optional, it succeeds: qobject_input_optional()
checks both the regular and the aliased input, finds neither, and
returns false. Still fine.
What if "data" is optional, too? Hmmm...
Example:
{ 'struct': 'Outer',
'data': { '*inner': 'Inner' },
{ 'struct': 'Inner',
'data': { '*true-name': 'str' } }
For input {}, we get an Outer object with
.has_inner = false,
.inner = NULL
Now add
'aliases': [ { 'name': 'alias-name',
'source': [ 'inner', 'true-name' ] } ] }
What do we get now? Is it
.has_inner = true,
.inner = { .has_true_name = false,
.true_name = NULL }
perhaps?
I'll study the rest of your reply next.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
2021-02-18 13:39 ` Markus Armbruster
@ 2021-02-18 16:10 ` Kevin Wolf
2021-02-19 9:11 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2021-02-18 16:10 UTC (permalink / raw)
To: Markus Armbruster; +Cc: jsnow, qemu-devel
Am 18.02.2021 um 14:39 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 17.02.2021 um 16:32 hat Markus Armbruster geschrieben:
> >> 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>
> >>
> >> Looking just at qobject_input_try_get_object() for now.
> >
> > :-(
> >
> > This patch doesn't even feel that complicated to me.
>
> I suspect it's just me having an unusually obtuse week.
>
> The code is straightforward enough. What I'm missing is a bit of "how
> does this accomplish the goal" and "why is this safe" here and there.
>
> > Old: Get the value from the QDict of the current StackObject with the
> > given name.
> >
> > New: First do alias resolution (with find_object_member), which results
> > in a StackObject and a name, and that's the QDict and key where you get
> > the value from.
>
> This part I understand.
>
> We simultaneously walk the QAPI type and the input QObject, consuming
> input as we go.
>
> Whenever the walk leaves a QAPI object type, we check the corresponding
> QObject has been consumed completely.
>
> With aliases, we additionally look for input in a certain enclosing
> input object (i.e. up the recursion stack). If found, consume.
>
> > Minor complication: Aliases can refer to members of nested objects that
> > may not be provided in the input. But we want these to work.
> >
> > For example, my chardev series defines aliases to flatten
> > SocketAddressLegacy (old syntax, I haven't rebased it yet):
> >
> > { 'union': 'SocketAddressLegacy',
> > 'data': {
> > 'inet': 'InetSocketAddress',
> > 'unix': 'UnixSocketAddress',
> > 'vsock': 'VsockSocketAddress',
> > 'fd': 'String' },
> > 'aliases': [
> > {'source': ['data']},
> > {'alias': 'fd', 'source': ['data', 'str']}
> > ]}
> >
> > Of course, the idea is that this input should work:
> >
> > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234' }
> >
> > However, without implicit objects, parsing 'data' fails because it
> > expects an object, but none is given (we specified all of its members on
> > the top level through aliases). What we would have to give is:
> >
> > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234', 'data': {} }
> >
> > And _that_ would work. Visiting 'data' succeeds because we now have the
> > object that the visitor expects, and when visiting its members, the
> > aliases fill in all of the mandatory values.
> >
> > So what this patch does is to implicitly assume the 'data': {} instead
> > of erroring out when we know that aliases exist that can still provide
> > values for the content of 'data'.
>
> Aliases exist than can still provide, but will they? What if input is
>
> {"type": "inet"}
>
> ?
>
> Your explanation makes me guess this is equivalent to
>
> {"type": "inet", "data": {}}
>
> which fails the visit, because mandatory members of "data" are missing.
> Fine.
Okay, if you want the gory details, then the answer is yes for this
case, but it depends.
If we're aliasing a single member, then we can easily check whether the
alias is actually specified. If it's not in the input, no implicit
object.
But in our example, it is a wildcard alias and we don't know yet which
aliases it will use. This depends on what the visitor for the implicit
object will do (future tense).
> If we make the members optional, it succeeds: qobject_input_optional()
> checks both the regular and the aliased input, finds neither, and
> returns false. Still fine.
>
> What if "data" is optional, too? Hmmm...
Yes, don't use optional objects in the middle of the path of a wildcard
alias unless there is no semantic difference between empty object and
absent object. This is documented in the code, but it might actually
still be missing from qapi-code-gen.txt.
> Example:
>
> { 'struct': 'Outer',
> 'data': { '*inner': 'Inner' },
>
> { 'struct': 'Inner',
> 'data': { '*true-name': 'str' } }
>
> For input {}, we get an Outer object with
>
> .has_inner = false,
> .inner = NULL
>
> Now add
>
> 'aliases': [ { 'name': 'alias-name',
> 'source': [ 'inner', 'true-name' ] } ] }
>
> What do we get now? Is it
>
> .has_inner = true,
> .inner = { .has_true_name = false,
> .true_name = NULL }
>
> perhaps?
I think this is the result you would get if you had used a wildcard
alias. But since you used a single-member alias, we would see that
'alias-name' is not in the input and actually still return the original
result:
.has_inner = false,
.inner = NULL
Kevin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
2021-02-18 16:10 ` Kevin Wolf
@ 2021-02-19 9:11 ` Markus Armbruster
2021-02-19 13:07 ` Markus Armbruster
0 siblings, 1 reply; 27+ messages in thread
From: Markus Armbruster @ 2021-02-19 9:11 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 18.02.2021 um 14:39 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Am 17.02.2021 um 16:32 hat Markus Armbruster geschrieben:
>> >> 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>
>> >>
>> >> Looking just at qobject_input_try_get_object() for now.
>> >
>> > :-(
>> >
>> > This patch doesn't even feel that complicated to me.
>>
>> I suspect it's just me having an unusually obtuse week.
>>
>> The code is straightforward enough. What I'm missing is a bit of "how
>> does this accomplish the goal" and "why is this safe" here and there.
>>
>> > Old: Get the value from the QDict of the current StackObject with the
>> > given name.
>> >
>> > New: First do alias resolution (with find_object_member), which results
>> > in a StackObject and a name, and that's the QDict and key where you get
>> > the value from.
>>
>> This part I understand.
>>
>> We simultaneously walk the QAPI type and the input QObject, consuming
>> input as we go.
>>
>> Whenever the walk leaves a QAPI object type, we check the corresponding
>> QObject has been consumed completely.
>>
>> With aliases, we additionally look for input in a certain enclosing
>> input object (i.e. up the recursion stack). If found, consume.
>>
>> > Minor complication: Aliases can refer to members of nested objects that
>> > may not be provided in the input. But we want these to work.
>> >
>> > For example, my chardev series defines aliases to flatten
>> > SocketAddressLegacy (old syntax, I haven't rebased it yet):
>> >
>> > { 'union': 'SocketAddressLegacy',
>> > 'data': {
>> > 'inet': 'InetSocketAddress',
>> > 'unix': 'UnixSocketAddress',
>> > 'vsock': 'VsockSocketAddress',
>> > 'fd': 'String' },
>> > 'aliases': [
>> > {'source': ['data']},
>> > {'alias': 'fd', 'source': ['data', 'str']}
>> > ]}
>> >
>> > Of course, the idea is that this input should work:
>> >
>> > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234' }
>> >
>> > However, without implicit objects, parsing 'data' fails because it
>> > expects an object, but none is given (we specified all of its members on
>> > the top level through aliases). What we would have to give is:
>> >
>> > { 'type': 'inet', 'hostname': 'localhost', 'port': '1234', 'data': {} }
>> >
>> > And _that_ would work. Visiting 'data' succeeds because we now have the
>> > object that the visitor expects, and when visiting its members, the
>> > aliases fill in all of the mandatory values.
>> >
>> > So what this patch does is to implicitly assume the 'data': {} instead
>> > of erroring out when we know that aliases exist that can still provide
>> > values for the content of 'data'.
>>
>> Aliases exist than can still provide, but will they? What if input is
>>
>> {"type": "inet"}
>>
>> ?
>>
>> Your explanation makes me guess this is equivalent to
>>
>> {"type": "inet", "data": {}}
>>
>> which fails the visit, because mandatory members of "data" are missing.
>> Fine.
>
> Okay, if you want the gory details, then the answer is yes for this
> case, but it depends.
I'm afraid I need the gory details to get over the review hump.
> If we're aliasing a single member, then we can easily check whether the
> alias is actually specified. If it's not in the input, no implicit
> object.
I figure this check is in the code parts I haven't gotten to, yet. Not
your fault.
> But in our example, it is a wildcard alias and we don't know yet which
> aliases it will use. This depends on what the visitor for the implicit
> object will do (future tense).
>
>> If we make the members optional, it succeeds: qobject_input_optional()
>> checks both the regular and the aliased input, finds neither, and
>> returns false. Still fine.
>>
>> What if "data" is optional, too? Hmmm...
>
> Yes, don't use optional objects in the middle of the path of a wildcard
> alias unless there is no semantic difference between empty object and
> absent object.
Aha! So my spidey-sense wasn't entirely wrong.
> This is documented in the code, but it might actually
> still be missing from qapi-code-gen.txt.
I can't find it there. Needs fixing, obviously.
I guess checking "path of a wildcard alias crosses optional objects" is
hard (impractical?) for the same reasons checking "alias can't resolve"
is.
I'd expect "alias can't resolve" to be caused by typos, incomplete
renames, and such. Basic testing should catch at least the typos. Not
ideal, but I guess it'll do, at least for now.
Relying on testing to catch "crosses optional objects" mistakes feels
iffier to me, because it takes more careful tests.
Ham-fisted way to make basic tests catch it: *ignore* optional objects
when resolving aliases. Is this a bad idea?
>> Example:
>>
>> { 'struct': 'Outer',
>> 'data': { '*inner': 'Inner' },
>>
>> { 'struct': 'Inner',
>> 'data': { '*true-name': 'str' } }
>>
>> For input {}, we get an Outer object with
>>
>> .has_inner = false,
>> .inner = NULL
>>
>> Now add
>>
>> 'aliases': [ { 'name': 'alias-name',
>> 'source': [ 'inner', 'true-name' ] } ] }
>>
>> What do we get now? Is it
>>
>> .has_inner = true,
>> .inner = { .has_true_name = false,
>> .true_name = NULL }
>>
>> perhaps?
>
> I think this is the result you would get if you had used a wildcard
> alias. But since you used a single-member alias, we would see that
> 'alias-name' is not in the input and actually still return the original
> result:
>
> .has_inner = false,
> .inner = NULL
I wasn't aware there's a difference. Now I am.
Thanks!
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
2021-02-19 9:11 ` Markus Armbruster
@ 2021-02-19 13:07 ` Markus Armbruster
0 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-19 13:07 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Markus Armbruster <armbru@redhat.com> writes:
> Kevin Wolf <kwolf@redhat.com> writes:
[...]
>> Yes, don't use optional objects in the middle of the path of a wildcard
>> alias unless there is no semantic difference between empty object and
>> absent object.
>
> Aha! So my spidey-sense wasn't entirely wrong.
Like optional members, union branches get visited only when the input is
shaped a certain way. Which makes me wonder: does "don't use optional
in the middle" apply to union branches, too?
Hmm, I figure it doesn't because
* If the union is flat, there is no object: the variant members are the
members of the branch struct type.
* If the union is simple, there is, but it's always there: 'data'.
Hope I'm not speaking in riddles.
>> This is documented in the code, but it might actually
>> still be missing from qapi-code-gen.txt.
>
> I can't find it there. Needs fixing, obviously.
"there" = qapi-code-gen.txt
> I guess checking "path of a wildcard alias crosses optional objects" is
> hard (impractical?) for the same reasons checking "alias can't resolve"
> is.
>
> I'd expect "alias can't resolve" to be caused by typos, incomplete
> renames, and such. Basic testing should catch at least the typos. Not
> ideal, but I guess it'll do, at least for now.
>
> Relying on testing to catch "crosses optional objects" mistakes feels
> iffier to me, because it takes more careful tests.
>
> Ham-fisted way to make basic tests catch it: *ignore* optional objects
> when resolving aliases. Is this a bad idea?
[...]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
2021-02-11 18:31 ` [PATCH v2 4/6] qapi: Apply aliases " Kevin Wolf
2021-02-17 15:32 ` Markus Armbruster
@ 2021-02-19 14:42 ` Markus Armbruster
2021-02-24 8:28 ` Markus Armbruster
2 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-19 14:42 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>
> ---
> qapi/qobject-input-visitor.c | 214 +++++++++++++++++++++++++++++++++--
> 1 file changed, 205 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index dd04ef0027..3ea5e5abd6 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -95,6 +95,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;
> @@ -167,9 +169,178 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> return full_name_so(qiv, name, false, tos);
> }
>
> +static bool find_object_member(QObjectInputVisitor *qiv,
> + StackObject **so, const char **name,
> + bool *implicit_object, Error **errp);
> +
> +/*
> + * Check whether the alias member defined by @a is present in the
> + * input and can be used to obtain the value for the member @name in
> + * the currently visited object.
> + */
> +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->name) {
> + name = a->name;
> + }
The contract promises "for the member @name", but it's actually for
a->name or else name.
Can a->name be non-null and different from name?
Possible way to avoid the complication:
static bool input_present(QObjectInputVisitor *qiv,
StackObject *so, char *name)
From now on: we're looking for input (@so, name), and @so is in
qiv->stack.
> +
> + /*
> + * Check whether the alias member is present in the input
> + * (possibly recursively because aliases are transitive).
Is the recursion guaranteed to terminate?
> + */
> + 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;
> + }
so->h has the unvisited keys.
qobject_input_try_get_object() removes from so->h when its argument
@consume is true. It is except in qobject_input_optional() and
qobject_input_start_alternate(). Both normally guard a call that passes
true.
The check here makes us return false when the input has been consumed
already: either by visiting it directly, or visiting something that
successfully aliased to it.
> +
> + return true;
We return true only when the input exists and has not been consumed
already. Okay.
If we ask alias_present() again before we get around to visiting (and
removing from so->h), it'll return true again. Can this happen?
> +}
I just realized:
0. An alias connects a member name in an outer object to one in an inner
object nested in the outer object (or multiple names for wildcards).
1. In visit_define_alias() and InputVisitorAlias, @name is the outer
member name, and source[] the inner member name.
2. We resolve aliases when we visit the inner member. Now the "source"
is the outer member.
3. Brain hurz!
> +
> +/*
> + * Check whether the member @name in the object visited by @so can be
> + * specified in the input by using the alias described by @a.
> + *
> + * If @name is only a prefix of the alias source, but doesn't match
> + * immediately, false is returned and @implicit_object is set to true
> + * if it is non-NULL. In all other cases, @implicit_object is left
> + * unchanged.
*implicit_object, to be precise.
> + */
How is @a related to @so? Peeking ahead... looks like it's a member of
so->aliases. Recommend to spell that out.
> +static bool alias_source_matches(QObjectInputVisitor *qiv,
> + StackObject *so, InputVisitorAlias *a,
> + const char *name, bool *implicit_object)
> +{
> + if (a->src[0] == NULL) {
> + assert(a->name == NULL);
This is a wildcard alias propagated to @so. Since there's nothing left
in ->src[], the alias's inner members are the members of QDict so->obj.
> + return true;
> + }
> +
> + if (!strcmp(a->src[0], name)) {
> + if (a->name && a->src[1] == NULL) {
This is a non-wildcard alias (possibly propagated, doesn't matter).
Since ->src[] contains just @name, the alias's inner member is the
member @name of so->obj.
> + /*
> + * We're matching an exact member, the source for this alias is
> + * immediately in @so.
> + */
> + return true;
Else, its inner member must be deeper still in so->obj.
> + } 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.
This is the clue I missed yesterday.
An alias can only be used if we visit its inner member. The caller
wants to fake input as needed to force the visit, but it needs help to
decide when to fake. Passing non-null @implicit_object asks for this
help.
> + */
> + if (!a->name || alias_present(qiv, a, a->name)) {
> + *implicit_object = true;
> + }
Cases:
0. Wildcard, we don't know about input
Set to true. We may be able to use the alias, so we better force a
visit.
1. Non-wildcard, have unconsumed input
Set to true. We will use the alias. But why do we need to force a
visit? I'm probably confused again/
2. Non-wildcard, don't have unconsumed input
Do nothing. We definitely can't use the alias, so there is no need
to force a visit.
> + }
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Find the place in the input where the value for the object member
> + * @name in @so is specified, considering applicable aliases.
> + *
> + * If a value could be found, true is returned and @so and @name are
> + * updated to identify the key name and StackObject where the value
> + * can be found in the input. (This is either unchanged or the
> + * alias_so/name of an alias.) The value of @implicit_object on
> + * return is undefined in this case.
> + *
> + * If no value could be found in the input, false is returned. This
> + * is not an error and @errp remains unchanged. If @implicit_object
> + * is non-NULL, it is set to true if the given name is a prefix of the
> + * source path of an alias for which a value may be present in the
> + * input. It is set to false otherwise.
> + *
> + * If an error occurs (e.g. two values are specified for the member
> + * through different names), false is returned and @errp is set. The
> + * value of @implicit_object on return is undefined in this case.
> + */
> +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->name from a->alias_so.
> + */
> + QSLIST_FOREACH(a, &cur_so->aliases, next) {
> + if (a->name == NULL && found) {
> + /*
> + * Skip wildcard aliases if we already have a match. This is
> + * not a conflict that should result in an error.
> + */
What if multiple wildcard aliases match?
Out of steam with the finishing line in sight. So it goes.
> + 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, false, *so));
> + return false;
> + } else {
> + found = a->name ?: *name;
> + *so = a->alias_so;
> + found_is_wildcard = !a->name;
> + }
> + }
> +
> + /* 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;
> @@ -187,10 +358,30 @@ 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) {
> + /*
> + * The member is not present in the input, but
> + * something inside of it might still be given through
> + * an alias. Pretend there was an empty object in the
> + * input.
> + */
> + 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 {
> @@ -216,9 +407,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;
> @@ -799,13 +991,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;
> }
>
> @@ -820,6 +1015,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] 27+ messages in thread
* Re: [PATCH v2 4/6] qapi: Apply aliases in qobject-input-visitor
2021-02-11 18:31 ` [PATCH v2 4/6] qapi: Apply aliases " Kevin Wolf
2021-02-17 15:32 ` Markus Armbruster
2021-02-19 14:42 ` Markus Armbruster
@ 2021-02-24 8:28 ` Markus Armbruster
2 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-24 8:28 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>
> ---
> qapi/qobject-input-visitor.c | 214 +++++++++++++++++++++++++++++++++--
> 1 file changed, 205 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index dd04ef0027..3ea5e5abd6 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -95,6 +95,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;
> @@ -167,9 +169,178 @@ static const char *full_name(QObjectInputVisitor *qiv, const char *name)
> return full_name_so(qiv, name, false, tos);
> }
>
> +static bool find_object_member(QObjectInputVisitor *qiv,
> + StackObject **so, const char **name,
> + bool *implicit_object, Error **errp);
> +
> +/*
> + * Check whether the alias member defined by @a is present in the
> + * input and can be used to obtain the value for the member @name in
> + * the currently visited object.
> + */
> +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->name) {
> + name = a->name;
> + }
> +
> + /*
> + * Check whether the alias member is present in the input
> + * (possibly recursively because aliases are transitive).
> + */
> + 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;
> +}
> +
> +/*
> + * Check whether the member @name in the object visited by @so can be
> + * specified in the input by using the alias described by @a.
> + *
> + * If @name is only a prefix of the alias source, but doesn't match
> + * immediately, false is returned and @implicit_object is set to true
> + * if it is non-NULL. In all other cases, @implicit_object is left
> + * unchanged.
> + */
> +static bool alias_source_matches(QObjectInputVisitor *qiv,
> + StackObject *so, InputVisitorAlias *a,
> + const char *name, bool *implicit_object)
> +{
> + if (a->src[0] == NULL) {
> + assert(a->name == NULL);
> + return true;
> + }
> +
> + if (!strcmp(a->src[0], name)) {
> + if (a->name && 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->name || alias_present(qiv, a, a->name)) {
> + *implicit_object = true;
> + }
> + }
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Find the place in the input where the value for the object member
> + * @name in @so is specified, considering applicable aliases.
> + *
> + * If a value could be found, true is returned and @so and @name are
> + * updated to identify the key name and StackObject where the value
> + * can be found in the input. (This is either unchanged or the
> + * alias_so/name of an alias.) The value of @implicit_object on
> + * return is undefined in this case.
> + *
> + * If no value could be found in the input, false is returned. This
> + * is not an error and @errp remains unchanged. If @implicit_object
> + * is non-NULL, it is set to true if the given name is a prefix of the
> + * source path of an alias for which a value may be present in the
> + * input. It is set to false otherwise.
> + *
> + * If an error occurs (e.g. two values are specified for the member
> + * through different names), false is returned and @errp is set. The
> + * value of @implicit_object on return is undefined in this case.
> + */
> +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->name from a->alias_so.
> + */
> + QSLIST_FOREACH(a, &cur_so->aliases, next) {
After looking at the complete loop, I understand:
@found is non-null if we have input, either non-alias input found
above, or alias input found below.
@found_is_wildcard is true if that input is from a wildcard alias.
If @found, *so is the alias's outer StackObject, i.e. where it was
defined. Else, @so is still the argument value. In either case,
@cur_so is the argument value.
If found, it is the member name in so->obj. It'll be stored to
*name after the loop.
Updating *so in the loop and *name after the loop feels a bit
awkward. Using a new variable @found_so to go with @found might
come out nicer. No need for @cur_so then. See also "*so may
already be messed up" below.
> + if (a->name == NULL && found) {
> + /*
> + * Skip wildcard aliases if we already have a match. This is
> + * not a conflict that should result in an error.
> + */
> + continue;
Ignore wildcard alias when we have other input.
If multiple different wildcard aliases apply, we silently ignore all but
the first one. Can this happen?
> + }
> +
> + if (!alias_source_matches(qiv, cur_so, a, *name, implicit_object)) {
The input for this alias is deeper. *implicit_object is now true if a
visit of @name needs to be forced to maybe resolve it. See
qobject_input_try_get_object() below.
> + continue;
> + }
Cases:
1. This is a wildcard alias propagated to @cur_so, i.e. the member *name
of cur_so->obj can serve as input.
2. This is a non-wildcard alias, and the member of cur_so->obj named by
it can serve as input, if it exists.
The alias_present() below uses *name when a is a wildcard, else a->name.
So this checks whether the member we want for input exists.
> +
> + if (!alias_present(qiv, a, *name)) {
> + continue;
It doesn't exist; ignore the alias.
> + }
It exists; the alias applies.
> +
> + if (found && !found_is_wildcard) {
The alias applies, but we already found non-alias input or non-wildcard
alias input. This is an error:
> + error_setg(errp, "Value for parameter %s was already given "
> + "through an alias",
> + full_name_so(qiv, *name, false, *so));
> + return false;
Note: *so may already be messed up here. I guess (hope?) the callers
are fine with that. The contract should spell it out, though. The
issue goes away if you store to *so only after the loop.
> + } else {
The alias applies, and nothing or only wildcard aliases applied before.
In the latter case, this alias is not a wildcard, because we skipped
those above.
Taken together:
* More than one input is an error regardless of where it comes from
(direct or alias), except
* we silently ignore extra wildcard alias input.
The error makes sense.
Ignoring one wildcard alias when there is non-wildcard input makes
sense.
When multiple wildcard aliases clash, I think we use the first one, and
ignore the others. I'm not sure that's what we want.
> + found = a->name ?: *name;
> + *so = a->alias_so;
> + found_is_wildcard = !a->name;
Record the alias applies. Update *so, but not *name.
> + }
> + }
> +
> + /* 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);
I'm sure there is a reason why we don't need to know whatever
@implicit_object can tell us, but it escapes me at the moment.
> + }
> +
> + *name = found;
We zap *name when nothing was found. I guess the callers are fine with
that. Nevertheless, the contract should spell it out. Storing only if
found might be simpler.
Oh, the caller right above might actually rely on this behavior. If
that's the case, more reason to spell it out in the contract.
> + return found;
> +}
> +
> static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
> const char *name,
> - bool consume)
> + bool consume, Error **errp)
> {
> StackObject *tos;
> QObject *qobj;
> @@ -187,10 +358,30 @@ 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) {
> + /*
> + * The member is not present in the input, but
> + * something inside of it might still be given through
> + * an alias. Pretend there was an empty object in the
> + * input.
> + */
> + 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 {
> @@ -216,9 +407,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;
> @@ -799,13 +991,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;
> }
>
> @@ -820,6 +1015,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] 27+ messages in thread
* Re: [PATCH v2 0/6] qapi: Add support for aliases
2021-02-11 18:31 [PATCH v2 0/6] qapi: Add support for aliases Kevin Wolf
` (5 preceding siblings ...)
2021-02-11 18:31 ` [PATCH v2 6/6] tests/qapi-schema: Test cases " Kevin Wolf
@ 2021-02-24 8:45 ` Markus Armbruster
6 siblings, 0 replies; 27+ messages in thread
From: Markus Armbruster @ 2021-02-24 8:45 UTC (permalink / raw)
To: Kevin Wolf; +Cc: jsnow, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> 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-v2
I'm done. Summary:
PATCH 1: Update to the big comment is still missing, but we can do that
last. I also flagged a possible contract clarification.
PATCH 2: Nits.
PATCH 3: Commit message questions.
PATCH 4: This one gave me trouble, and I feel unable to summarize. I'm
afraid you have to wade through my review to answer questions and
address issues as far as practical. And then we try again with v3.
PATCH 5: Documentation tweaks.
PATCH 6: Please consider adding more positive tests, as discussed in
review of PATCH 5.
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2021-02-24 8:48 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-11 18:31 [PATCH v2 0/6] qapi: Add support for aliases Kevin Wolf
2021-02-11 18:31 ` [PATCH v2 1/6] qapi: Add interfaces for alias support to Visitor Kevin Wolf
2021-02-16 11:56 ` Markus Armbruster
2021-02-11 18:31 ` [PATCH v2 2/6] qapi: Remember alias definitions in qobject-input-visitor Kevin Wolf
2021-02-16 12:06 ` Markus Armbruster
2021-02-11 18:31 ` [PATCH v2 3/6] qapi: Simplify full_name_nth() " Kevin Wolf
2021-02-16 12:22 ` Markus Armbruster
2021-02-11 18:31 ` [PATCH v2 4/6] qapi: Apply aliases " Kevin Wolf
2021-02-17 15:32 ` Markus Armbruster
2021-02-17 17:50 ` Kevin Wolf
2021-02-18 13:39 ` Markus Armbruster
2021-02-18 16:10 ` Kevin Wolf
2021-02-19 9:11 ` Markus Armbruster
2021-02-19 13:07 ` Markus Armbruster
2021-02-19 14:42 ` Markus Armbruster
2021-02-24 8:28 ` Markus Armbruster
2021-02-11 18:31 ` [PATCH v2 5/6] qapi: Add support for aliases Kevin Wolf
2021-02-16 15:43 ` Markus Armbruster
2021-02-17 15:23 ` Markus Armbruster
2021-02-17 16:17 ` Kevin Wolf
2021-02-18 10:26 ` Markus Armbruster
2021-02-11 18:31 ` [PATCH v2 6/6] tests/qapi-schema: Test cases " Kevin Wolf
2021-02-16 15:14 ` Markus Armbruster
2021-02-16 15:31 ` Kevin Wolf
2021-02-16 16:14 ` Markus Armbruster
2021-02-17 12:23 ` Markus Armbruster
2021-02-24 8:45 ` [PATCH v2 0/6] qapi: Add support " 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.