qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] qapi: introduce forwarding visitor


From: Paolo Bonzini
Subject: Re: [PATCH 1/2] qapi: introduce forwarding visitor
Date: Thu, 22 Jul 2021 17:08:19 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

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 <pbonzini@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);




reply via email to

[Prev in Thread] Current Thread [Next in Thread]