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

[Paolo Bonzini <pbonzini@redhat.com>]

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