On 22/07/21 16:02, Markus Armbruster wrote:
Double-checking: the other fields are not accessible via this visitor.
Correct?
Correct.
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
include/qapi/forward-visitor.h | 27 +++
qapi/meson.build | 1 +
qapi/qapi-forward-visitor.c | 307 ++++++++++++++++++++++++++++++
tests/unit/meson.build | 1 +
tests/unit/test-forward-visitor.c | 165 ++++++++++++++++
5 files changed, 501 insertions(+)
create mode 100644 include/qapi/forward-visitor.h
create mode 100644 qapi/qapi-forward-visitor.c
create mode 100644 tests/unit/test-forward-visitor.c
Missing: update of the big comment in include/qapi/visitor.h. Can be
done on top.
Also because I'm not sure what to add. :)
This is not a fifth type of visitor, it's a wrapper for the existing types (two
of them, input and output; the other two don't break horribly but make no sense
either).
+static bool forward_field_translate_name(ForwardFieldVisitor *v, const char
**name,
+ Error **errp)
+{
+ if (v->depth) {
+ return true;
+ }
Succeed when we're in a sub-struct.
+ if (g_str_equal(*name, v->from)) {
+ *name = v->to;
+ return true;
+ }
Succeed when we're in the root struct and @name is the alias name.
Replace the alias name by the real one.
+ error_setg(errp, QERR_MISSING_PARAMETER, *name);
+ return false;
Fail when we're in the root struct and @name is not the alias name.
+}
Can you explain why you treat names in sub-structs differently than
names other than the alias name in the root struct?
Taking the example of QOM alias properties, if the QOM property you're
aliasing is a struct, its field names are irrelevant. The caller may
not even know what they are, as they are not part of the namespace (e.g.
the toplevel QDict returned by keyval_parse) that is being modified.
There are no aliased compound QOM properties that I can make a proper example
with, unfortunately.
+ /*
+ * The name of alternates is reused when accessing the content,
+ * so do not increase depth here.
+ */
I understand why you don't increase @depth here (same reason
qobject-input-visitor.c doesn't qobject_input_push() here). I don't
understand the comment :)
See above: the alternate is not a struct; the names that are passed
between start_alternate and end_alternate are within the same namespace
as the toplevel field.
As to the comment, the idea is: if those calls used e.g. name == NULL,
the depth would need to be increased, but the name will be the same one
that was received by start_alternate. Change to "The name passed to
start_alternate is also used when accessing the content"?
+Visitor *visitor_forward_field(Visitor *target, const char *from, const char
*to)
+{
+ ForwardFieldVisitor *v = g_new0(ForwardFieldVisitor, 1);
+
+ v->visitor.type = target->type;
Do arbitrary types work? Or is this limited to input and output
visitors?
They don't crash, but they don't make sense because 1) they should not
live outside qapi_clone and visit_free_* 2) they use NULL for the
outermost name.
Not forwarded: method .type_size(). Impact: visit_type_size() will call
the wrapped visitor's .type_uint64() instead of its .type_size(). The
two differ for the opts visitor, the keyval input visitor, the string
input visitor, and the string output visitor.
Fixed, of course. Incremental diff after my sig.
Paolo
diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c
index f04f72f66d..a4b111e22a 100644
--- a/qapi/qapi-forward-visitor.c
+++ b/qapi/qapi-forward-visitor.c
@@ -57,6 +57,7 @@ static bool forward_field_translate_name(ForwardFieldVisitor
*v, const char **na
static bool forward_field_check_struct(Visitor *v, Error **errp)
{
ForwardFieldVisitor *ffv = to_ffv(v);
+
return visit_check_struct(ffv->target, errp);
}
@@ -78,6 +79,7 @@ static bool forward_field_start_struct(Visitor *v, const char *name, void **obj,
static void forward_field_end_struct(Visitor *v, void **obj)
{
ForwardFieldVisitor *ffv = to_ffv(v);
+
assert(ffv->depth);
ffv->depth--;
visit_end_struct(ffv->target, obj);
@@ -132,8 +134,8 @@ static bool forward_field_start_alternate(Visitor *v, const
char *name,
return false;
}
/*
- * The name of alternates is reused when accessing the content,
- * so do not increase depth here.
+ * The name passed to start_alternate is used also in the visit_type_*
calls
+ * that retrieve the alternate's content; so, do not increase depth here.
*/
return visit_start_alternate(ffv->target, name, obj, size, errp);
}
@@ -189,6 +191,17 @@ static bool forward_field_type_str(Visitor *v, const char
*name, char **obj,
return visit_type_str(ffv->target, name, obj, errp);
}
+static bool forward_field_type_size(Visitor *v, const char *name, uint64_t *obj,
+ Error **errp)
+{
+ ForwardFieldVisitor *ffv = to_ffv(v);
+
+ if (!forward_field_translate_name(ffv, &name, errp)) {
+ return false;
+ }
+ return visit_type_size(ffv->target, name, obj, errp);
+}
+
static bool forward_field_type_number(Visitor *v, const char *name, double
*obj,
Error **errp)
{
@@ -275,6 +288,12 @@ Visitor *visitor_forward_field(Visitor *target, const char
*from, const char *to
{
ForwardFieldVisitor *v = g_new0(ForwardFieldVisitor, 1);
+ /*
+ * Clone and dealloc visitors don't use a name for the toplevel
+ * visit, so they make no sense here.
+ */
+ assert(target->type == VISITOR_OUTPUT || target->type == VISITOR_INPUT);
+
v->visitor.type = target->type;
v->visitor.start_struct = forward_field_start_struct;
v->visitor.check_struct = forward_field_check_struct;
@@ -285,18 +304,19 @@ Visitor *visitor_forward_field(Visitor *target, const
char *from, const char *to
v->visitor.end_list = forward_field_end_list;
v->visitor.start_alternate = forward_field_start_alternate;
v->visitor.end_alternate = forward_field_end_alternate;
- v->visitor.optional = forward_field_optional;
- v->visitor.deprecated_accept = forward_field_deprecated_accept;
- v->visitor.deprecated = forward_field_deprecated;
- v->visitor.free = forward_field_free;
v->visitor.type_int64 = forward_field_type_int64;
v->visitor.type_uint64 = forward_field_type_uint64;
+ v->visitor.type_size = forward_field_type_size;
v->visitor.type_bool = forward_field_type_bool;
v->visitor.type_str = forward_field_type_str;
v->visitor.type_number = forward_field_type_number;
v->visitor.type_any = forward_field_type_any;
v->visitor.type_null = forward_field_type_null;
+ v->visitor.optional = forward_field_optional;
+ v->visitor.deprecated_accept = forward_field_deprecated_accept;
+ v->visitor.deprecated = forward_field_deprecated;
v->visitor.complete = forward_field_complete;
+ v->visitor.free = forward_field_free;
v->target = target;
v->from = g_strdup(from);
diff --git a/tests/unit/test-forward-visitor.c
b/tests/unit/test-forward-visitor.c
index 32ba359977..0de43964d2 100644
--- a/tests/unit/test-forward-visitor.c
+++ b/tests/unit/test-forward-visitor.c
@@ -69,6 +69,33 @@ static void test_forward_any(void)
qapi_free_UserDefOne(dst);
}
+static void test_forward_size(void)
+{
+ /*
+ * visit_type_size does not return a pointer, so visit_with_forward
+ * cannot be used.
+ */
+ bool help = false;
+ QDict *src = keyval_parse("src=1.5M", NULL, &help, &error_abort);
+ Visitor *v, *alias_v;
+ Error *err = NULL;
+ uint64_t result = 0;
+
+ v = qobject_input_visitor_new_keyval(QOBJECT(src));
+ visit_start_struct(v, NULL, NULL, 0, &error_abort);
+
+ alias_v = visitor_forward_field(v, "dst", "src");
+ visit_type_size(alias_v, "src", &result, &err);
+ error_free_or_abort(&err);
+ visit_type_size(alias_v, "dst", &result, &err);
+ assert(result == 3 << 19);
+ assert(err == NULL);
+ visit_free(alias_v);
+
+ visit_end_struct(v, NULL);
+ visit_free(v);
+}
+
static void test_forward_number(void)
{
/*
@@ -157,6 +184,7 @@ int main(int argc, char **argv)
g_test_add_func("/visitor/forward/struct", test_forward_struct);
g_test_add_func("/visitor/forward/alternate", test_forward_alternate);
g_test_add_func("/visitor/forward/string", test_forward_string);
+ g_test_add_func("/visitor/forward/size", test_forward_size);
g_test_add_func("/visitor/forward/number", test_forward_number);
g_test_add_func("/visitor/forward/any", test_forward_any);
g_test_add_func("/visitor/forward/list", test_forward_list);
