[PATCH 0/2] qapi/qom: use correct field name when getting/setting alias properties

[PATCH 0/2] qapi/qom: use correct field name when getting/setting alias properties

[Paolo Bonzini]

Switching -M parsing from QemuOptions and StringInputVisitor to keyval and
QObjectInputVisitor exposed a latent bug in alias properties.
Alias targets have a different name than the alias property itself
(e.g. a machine's pflash0 might be an alias of a property named 'drive').
When the target's getter or setter invokes the visitor, it will use
the "wrong" name compared to what was passed on the command line,
and the visitor will not be able to find it.

The solution that is implemented here is for alias getters and setters
to wrap the incoming visitor, and forward the sole field that the target
is expecting while renaming it appropriately.

Patch 1 implements the visitor adapter, while patch 2 applies it in QOM.

Paolo


Paolo Bonzini (2):
  qapi: introduce forwarding visitor
  qom: use correct field name when getting/setting alias properties

 include/qapi/forward-visitor.h    |  27 +++
 qapi/meson.build                  |   1 +
 qapi/qapi-forward-visitor.c       | 307 ++++++++++++++++++++++++++++++
 qom/object.c                      |   9 +-
 tests/unit/meson.build            |   1 +
 tests/unit/test-forward-visitor.c | 165 ++++++++++++++++
 6 files changed, 508 insertions(+), 2 deletions(-)
 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

-- 
2.31.1

[PATCH 1/2] qapi: introduce forwarding visitor

[Paolo Bonzini]

This new adaptor visitor takes a single field of the adaptee, and exposes it
with a different name.

This will be used for QOM alias properties.  Alias targets can of course
have a different name than the alias property itself (e.g. a machine's
pflash0 might be an alias of a property named 'drive').  When the target's
getter or setter invokes the visitor, it will use a different name than
what the caller expects, and the visitor will not be able to find it
(or will consume erroneously).

The solution is for alias getters and setters to wrap the incoming
visitor, and forward the sole field that the target is expecting while
renaming it appropriately.

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

diff --git a/include/qapi/forward-visitor.h b/include/qapi/forward-visitor.h
new file mode 100644
index 0000000000..c7002d53e6
--- /dev/null
+++ b/include/qapi/forward-visitor.h
@@ -0,0 +1,27 @@
+/*
+ * Forwarding visitor
+ *
+ * Copyright Red Hat, Inc. 2021
+ *
+ * Author: Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef FORWARD_VISITOR_H
+#define FORWARD_VISITOR_H
+
+#include "qapi/visitor.h"
+
+typedef struct ForwardFieldVisitor ForwardFieldVisitor;
+
+/*
+ * The forwarding visitor only expects a single name, @from, to be passed for
+ * toplevel fields.  It is converted to @to and forward to the @target visitor.
+ * Calls within a struct are forwarded without changing the name.
+ */
+Visitor *visitor_forward_field(Visitor *target, const char *from, const char *to);
+
+#endif
diff --git a/qapi/meson.build b/qapi/meson.build
index 376f4ceafe..c356a385e3 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -2,6 +2,7 @@ util_ss.add(files(
   'opts-visitor.c',
   'qapi-clone-visitor.c',
   'qapi-dealloc-visitor.c',
+  'qapi-forward-visitor.c',
   'qapi-util.c',
   'qapi-visit-core.c',
   'qobject-input-visitor.c',
diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c
new file mode 100644
index 0000000000..bc6412d52e
--- /dev/null
+++ b/qapi/qapi-forward-visitor.c
@@ -0,0 +1,307 @@
+/*
+ * Forward Visitor
+ *
+ * Copyright (C) 2021 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include <math.h>
+#include "qapi/compat-policy.h"
+#include "qapi/error.h"
+#include "qapi/forward-visitor.h"
+#include "qapi/visitor-impl.h"
+#include "qemu/queue.h"
+#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qbool.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnull.h"
+#include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qstring.h"
+#include "qemu/cutils.h"
+#include "qemu/option.h"
+
+struct ForwardFieldVisitor {
+    Visitor visitor;
+
+    Visitor *target;
+    char *from;
+    char *to;
+
+    int depth;
+};
+
+static ForwardFieldVisitor *to_ffv(Visitor *v)
+{
+    return container_of(v, ForwardFieldVisitor, visitor);
+}
+
+static bool forward_field_translate_name(ForwardFieldVisitor *v, const char **name,
+                                         Error **errp)
+{
+    if (v->depth) {
+        return true;
+    }
+    if (g_str_equal(*name, v->from)) {
+        *name = v->to;
+        return true;
+    }
+    error_setg(errp, QERR_MISSING_PARAMETER, *name);
+    return false;
+}
+
+static bool forward_field_check_struct(Visitor *v, Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+    return visit_check_struct(ffv->target, errp);
+}
+
+static bool forward_field_start_struct(Visitor *v, const char *name, void **obj,
+                                       size_t size, Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, errp)) {
+        return false;
+    }
+    if (!visit_start_struct(ffv->target, name, obj, size, errp)) {
+        return false;
+    }
+    ffv->depth++;
+    return true;
+}
+
+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);
+}
+
+static bool forward_field_start_list(Visitor *v, const char *name,
+                                     GenericList **list, size_t size,
+                                     Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, errp)) {
+        return false;
+    }
+    ffv->depth++;
+    return visit_start_list(ffv->target, name, list, size, errp);
+}
+
+static GenericList *forward_field_next_list(Visitor *v, GenericList *tail,
+                                            size_t size)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    assert(ffv->depth);
+    return visit_next_list(ffv->target, tail, size);
+}
+
+static bool forward_field_check_list(Visitor *v, Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    assert(ffv->depth);
+    return visit_check_list(ffv->target, errp);
+}
+
+static void forward_field_end_list(Visitor *v, void **obj)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    assert(ffv->depth);
+    ffv->depth--;
+    visit_end_list(ffv->target, obj);
+}
+
+static bool forward_field_start_alternate(Visitor *v, const char *name,
+                                          GenericAlternate **obj, size_t size,
+                                          Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, errp)) {
+        return false;
+    }
+    /*
+     * The name of alternates is reused when accessing the content,
+     * so do not increase depth here.
+     */
+    return visit_start_alternate(ffv->target, name, obj, size, errp);
+}
+
+static void forward_field_end_alternate(Visitor *v, void **obj)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    visit_end_alternate(ffv->target, obj);
+}
+
+static bool forward_field_type_int64(Visitor *v, const char *name, int64_t *obj,
+                                     Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, errp)) {
+        return false;
+    }
+    return visit_type_int64(ffv->target, name, obj, errp);
+}
+
+static bool forward_field_type_uint64(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_uint64(ffv->target, name, obj, errp);
+}
+
+static bool forward_field_type_bool(Visitor *v, const char *name, bool *obj,
+                                    Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, errp)) {
+        return false;
+    }
+    return visit_type_bool(ffv->target, name, obj, errp);
+}
+
+static bool forward_field_type_str(Visitor *v, const char *name, char **obj,
+                                   Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, errp)) {
+        return false;
+    }
+    return visit_type_str(ffv->target, name, obj, errp);
+}
+
+static bool forward_field_type_number(Visitor *v, const char *name, double *obj,
+                                      Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, errp)) {
+        return false;
+    }
+    return visit_type_number(ffv->target, name, obj, errp);
+}
+
+static bool forward_field_type_any(Visitor *v, const char *name, QObject **obj,
+                                   Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, errp)) {
+        return false;
+    }
+    return visit_type_any(ffv->target, name, obj, errp);
+}
+
+static bool forward_field_type_null(Visitor *v, const char *name,
+                                    QNull **obj, Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, errp)) {
+        return false;
+    }
+    return visit_type_null(ffv->target, name, obj, errp);
+}
+
+static void forward_field_optional(Visitor *v, const char *name, bool *present)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, NULL)) {
+        *present = false;
+        return;
+    }
+    visit_optional(ffv->target, name, present);
+}
+
+static bool forward_field_deprecated_accept(Visitor *v, const char *name,
+                                            Error **errp)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, errp)) {
+        return false;
+    }
+    return visit_deprecated_accept(ffv->target, name, errp);
+}
+
+static bool forward_field_deprecated(Visitor *v, const char *name)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    if (!forward_field_translate_name(ffv, &name, NULL)) {
+        return false;
+    }
+    return visit_deprecated(ffv->target, name);
+}
+
+static void forward_field_complete(Visitor *v, void *opaque)
+{
+    /*
+     * Do nothing, the complete method will be called at due time
+     * on the target visitor.
+     */
+}
+
+static void forward_field_free(Visitor *v)
+{
+    ForwardFieldVisitor *ffv = to_ffv(v);
+
+    g_free(ffv->from);
+    g_free(ffv->to);
+    g_free(ffv);
+}
+
+Visitor *visitor_forward_field(Visitor *target, const char *from, const char *to)
+{
+    ForwardFieldVisitor *v = g_new0(ForwardFieldVisitor, 1);
+
+    v->visitor.type = target->type;
+    v->visitor.start_struct = forward_field_start_struct;
+    v->visitor.check_struct = forward_field_check_struct;
+    v->visitor.end_struct = forward_field_end_struct;
+    v->visitor.start_list = forward_field_start_list;
+    v->visitor.next_list = forward_field_next_list;
+    v->visitor.check_list = forward_field_check_list;
+    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_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.complete = forward_field_complete;
+
+    v->target = target;
+    v->from = g_strdup(from);
+    v->to = g_strdup(to);
+
+    return &v->visitor;
+}
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 3e0504dd21..5736d285b2 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -14,6 +14,7 @@ tests = {
   'test-qobject-output-visitor': [testqapi],
   'test-clone-visitor': [testqapi],
   'test-qobject-input-visitor': [testqapi],
+  'test-forward-visitor': [testqapi],
   'test-string-input-visitor': [testqapi],
   'test-string-output-visitor': [testqapi],
   'test-opts-visitor': [testqapi],
diff --git a/tests/unit/test-forward-visitor.c b/tests/unit/test-forward-visitor.c
new file mode 100644
index 0000000000..bab1844f7f
--- /dev/null
+++ b/tests/unit/test-forward-visitor.c
@@ -0,0 +1,165 @@
+/*
+ * QAPI Forwarding Visitor unit-tests.
+ *
+ * Copyright (C) 2021 Red Hat Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "qemu-common.h"
+#include "qapi/forward-visitor.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qobject.h"
+#include "qapi/qmp/qdict.h"
+#include "test-qapi-visit.h"
+#include "qemu/option.h"
+
+typedef bool GenericVisitor (Visitor *, const char *, void **, Error **);
+#define CAST_VISIT_TYPE(fn) ((GenericVisitor *)(fn))
+
+/*
+ * Parse @srcstr and wrap it with a ForwardFieldVisitor converting "src" to
+ * "dst". Check that visiting the result with "src" name fails, and return
+ * the result of visiting "dst".
+ */
+static void *visit_with_forward(const char *srcstr, GenericVisitor *fn)
+{
+    bool help = false;
+    QDict *src = keyval_parse(srcstr, NULL, &help, &error_abort);
+    Visitor *v, *alias_v;
+    Error *err = NULL;
+    void *result = NULL;
+
+    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");
+    fn(alias_v, "src", &result, &err);
+    error_free_or_abort(&err);
+    assert(!result);
+    fn(alias_v, "dst", &result, &err);
+    assert(err == NULL);
+    visit_free(alias_v);
+
+    visit_end_struct(v, NULL);
+    visit_free(v);
+    return result;
+}
+
+static void test_forward_any(void)
+{
+    QObject *src = visit_with_forward("src.integer=42,src.string=Hello,src.enum1=value2",
+                                      CAST_VISIT_TYPE(visit_type_any));
+    Visitor *v = qobject_input_visitor_new_keyval(src);
+    Error *err = NULL;
+    UserDefOne *dst;
+
+    visit_type_UserDefOne(v, NULL, &dst, &err);
+    assert(err == NULL);
+    visit_free(v);
+
+    g_assert_cmpint(dst->integer, ==, 42);
+    g_assert_cmpstr(dst->string, ==, "Hello");
+    g_assert_cmpint(dst->has_enum1, ==, true);
+    g_assert_cmpint(dst->enum1, ==, ENUM_ONE_VALUE2);
+    qapi_free_UserDefOne(dst);
+}
+
+static void test_forward_number(void)
+{
+    /*
+     * visit_type_number does not return a pointer, so visit_with_forward
+     * cannot be used.
+     */
+    bool help = false;
+    QDict *src = keyval_parse("src=1.5", NULL, &help, &error_abort);
+    Visitor *v, *alias_v;
+    Error *err = NULL;
+    double result = 0.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_number(alias_v, "src", &result, &err);
+    error_free_or_abort(&err);
+    visit_type_number(alias_v, "dst", &result, &err);
+    assert(result == 1.5);
+    assert(err == NULL);
+    visit_free(alias_v);
+
+    visit_end_struct(v, NULL);
+    visit_free(v);
+}
+
+static void test_forward_string(void)
+{
+    char *dst = visit_with_forward("src=Hello",
+                                   CAST_VISIT_TYPE(visit_type_str));
+
+    g_assert_cmpstr(dst, ==, "Hello");
+    g_free(dst);
+}
+
+static void test_forward_struct(void)
+{
+    UserDefOne *dst = visit_with_forward("src.integer=42,src.string=Hello",
+                                         CAST_VISIT_TYPE(visit_type_UserDefOne));
+
+    g_assert_cmpint(dst->integer, ==, 42);
+    g_assert_cmpstr(dst->string, ==, "Hello");
+    g_assert_cmpint(dst->has_enum1, ==, false);
+    qapi_free_UserDefOne(dst);
+}
+
+static void test_forward_alternate(void)
+{
+    AltStrObj *s_dst = visit_with_forward("src=hello",
+                                          CAST_VISIT_TYPE(visit_type_AltStrObj));
+    AltStrObj *o_dst = visit_with_forward("src.integer=42,src.boolean=true,src.string=world",
+                                          CAST_VISIT_TYPE(visit_type_AltStrObj));
+
+    g_assert_cmpint(s_dst->type, ==, QTYPE_QSTRING);
+    g_assert_cmpstr(s_dst->u.s, ==, "hello");
+    g_assert_cmpint(o_dst->type, ==, QTYPE_QDICT);
+    g_assert_cmpint(o_dst->u.o.integer, ==, 42);
+    g_assert_cmpint(o_dst->u.o.boolean, ==, true);
+    g_assert_cmpstr(o_dst->u.o.string, ==, "world");
+
+    qapi_free_AltStrObj(s_dst);
+    qapi_free_AltStrObj(o_dst);
+}
+
+static void test_forward_list(void)
+{
+    uint8List *dst = visit_with_forward("src.0=1,src.1=2,src.2=3,src.3=4",
+                                        CAST_VISIT_TYPE(visit_type_uint8List));
+    uint8List *tmp;
+    int i;
+
+    for (tmp = dst, i = 1; i <= 4; i++) {
+        g_assert(tmp);
+        g_assert_cmpint(tmp->value, ==, i);
+        tmp = tmp->next;
+    }
+    g_assert(!tmp);
+    qapi_free_uint8List(dst);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    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/number", test_forward_number);
+    g_test_add_func("/visitor/forward/any", test_forward_any);
+    g_test_add_func("/visitor/forward/list", test_forward_list);
+
+    return g_test_run();
+}
-- 
2.31.1

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

[Eric Blake]

On Mon, Jul 19, 2021 at 12:40:32PM +0200, Paolo Bonzini wrote:
> This new adaptor visitor takes a single field of the adaptee, and exposes it
> with a different name.
> 
> This will be used for QOM alias properties.  Alias targets can of course
> have a different name than the alias property itself (e.g. a machine's
> pflash0 might be an alias of a property named 'drive').  When the target's
> getter or setter invokes the visitor, it will use a different name than
> what the caller expects, and the visitor will not be able to find it
> (or will consume erroneously).

How does this differ from Kevin's attempt to add QMP aliasing,
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg04097.html

> 
> The solution is for alias getters and setters to wrap the incoming
> visitor, and forward the sole field that the target is expecting while
> renaming it appropriately.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

> diff --git a/include/qapi/forward-visitor.h b/include/qapi/forward-visitor.h
> new file mode 100644
> index 0000000000..c7002d53e6
> --- /dev/null
> +++ b/include/qapi/forward-visitor.h

> +
> +typedef struct ForwardFieldVisitor ForwardFieldVisitor;
> +
> +/*
> + * The forwarding visitor only expects a single name, @from, to be passed for
> + * toplevel fields.  It is converted to @to and forward to the @target visitor.

and forwarded

> + * Calls within a struct are forwarded without changing the name.
> + */
> +Visitor *visitor_forward_field(Visitor *target, const char *from, const char *to);
> +
> +#endif

> +++ b/qapi/qapi-forward-visitor.c
> @@ -0,0 +1,307 @@
> +/*
> + * Forward Visitor
> + *
> + * Copyright (C) 2021 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include <math.h>

Did you really use math.h?


> +
> +static void forward_field_complete(Visitor *v, void *opaque)
> +{
> +    /*
> +     * Do nothing, the complete method will be called at due time

s/at/in/

> +     * on the target visitor.
> +     */
> +}
> +

> +++ b/tests/unit/test-forward-visitor.c
> @@ -0,0 +1,165 @@
> +/*
> + * QAPI Forwarding Visitor unit-tests.

Otherwise it looks like it does what it advertises.  I don't know if
Markus will have any opinions, but I'm comfortable with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This new adaptor visitor takes a single field of the adaptee, and exposes it
> with a different name.
>
> This will be used for QOM alias properties.  Alias targets can of course
> have a different name than the alias property itself (e.g. a machine's
> pflash0 might be an alias of a property named 'drive').  When the target's
> getter or setter invokes the visitor, it will use a different name than
> what the caller expects, and the visitor will not be able to find it
> (or will consume erroneously).
>
> The solution is for alias getters and setters to wrap the incoming
> visitor, and forward the sole field that the target is expecting while
> renaming it appropriately.

Double-checking: the other fields are not accessible via this visitor.
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.

>
> diff --git a/include/qapi/forward-visitor.h b/include/qapi/forward-visitor.h
> new file mode 100644
> index 0000000000..c7002d53e6
> --- /dev/null
> +++ b/include/qapi/forward-visitor.h
> @@ -0,0 +1,27 @@
> +/*
> + * Forwarding visitor
> + *
> + * Copyright Red Hat, Inc. 2021
> + *
> + * Author: Paolo Bonzini <pbonzini@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#ifndef FORWARD_VISITOR_H
> +#define FORWARD_VISITOR_H
> +
> +#include "qapi/visitor.h"
> +
> +typedef struct ForwardFieldVisitor ForwardFieldVisitor;
> +
> +/*
> + * The forwarding visitor only expects a single name, @from, to be passed for
> + * toplevel fields.  It is converted to @to and forward to the @target visitor.
> + * Calls within a struct are forwarded without changing the name.
> + */
> +Visitor *visitor_forward_field(Visitor *target, const char *from, const char *to);
> +
> +#endif
> diff --git a/qapi/meson.build b/qapi/meson.build
> index 376f4ceafe..c356a385e3 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -2,6 +2,7 @@ util_ss.add(files(
>    'opts-visitor.c',
>    'qapi-clone-visitor.c',
>    'qapi-dealloc-visitor.c',
> +  'qapi-forward-visitor.c',
>    'qapi-util.c',
>    'qapi-visit-core.c',
>    'qobject-input-visitor.c',
> diff --git a/qapi/qapi-forward-visitor.c b/qapi/qapi-forward-visitor.c
> new file mode 100644
> index 0000000000..bc6412d52e
> --- /dev/null
> +++ b/qapi/qapi-forward-visitor.c
> @@ -0,0 +1,307 @@
> +/*
> + * Forward Visitor
> + *
> + * Copyright (C) 2021 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include <math.h>
> +#include "qapi/compat-policy.h"
> +#include "qapi/error.h"
> +#include "qapi/forward-visitor.h"
> +#include "qapi/visitor-impl.h"
> +#include "qemu/queue.h"
> +#include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qbool.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qapi/qmp/qlist.h"
> +#include "qapi/qmp/qnull.h"
> +#include "qapi/qmp/qnum.h"
> +#include "qapi/qmp/qstring.h"
> +#include "qemu/cutils.h"
> +#include "qemu/option.h"
> +
> +struct ForwardFieldVisitor {
> +    Visitor visitor;
> +
> +    Visitor *target;
> +    char *from;
> +    char *to;
> +
> +    int depth;
> +};

Comment the members?  In particular @depth.

> +
> +static ForwardFieldVisitor *to_ffv(Visitor *v)
> +{
> +    return container_of(v, ForwardFieldVisitor, visitor);
> +}
> +
> +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?

> +
> +static bool forward_field_check_struct(Visitor *v, Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);

Humor me: blank line between declarations and statements.

> +    return visit_check_struct(ffv->target, errp);
> +}
> +
> +static bool forward_field_start_struct(Visitor *v, const char *name, void **obj,
> +                                       size_t size, Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, errp)) {
> +        return false;
> +    }
> +    if (!visit_start_struct(ffv->target, name, obj, size, errp)) {
> +        return false;
> +    }
> +    ffv->depth++;
> +    return true;
> +}
> +
> +static void forward_field_end_struct(Visitor *v, void **obj)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);

Humor me: blank line between declarations and statements.

> +    assert(ffv->depth);
> +    ffv->depth--;
> +    visit_end_struct(ffv->target, obj);
> +}
> +
> +static bool forward_field_start_list(Visitor *v, const char *name,
> +                                     GenericList **list, size_t size,
> +                                     Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, errp)) {
> +        return false;
> +    }
> +    ffv->depth++;
> +    return visit_start_list(ffv->target, name, list, size, errp);
> +}
> +
> +static GenericList *forward_field_next_list(Visitor *v, GenericList *tail,
> +                                            size_t size)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    assert(ffv->depth);
> +    return visit_next_list(ffv->target, tail, size);
> +}
> +
> +static bool forward_field_check_list(Visitor *v, Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    assert(ffv->depth);
> +    return visit_check_list(ffv->target, errp);
> +}
> +
> +static void forward_field_end_list(Visitor *v, void **obj)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    assert(ffv->depth);
> +    ffv->depth--;
> +    visit_end_list(ffv->target, obj);
> +}
> +
> +static bool forward_field_start_alternate(Visitor *v, const char *name,
> +                                          GenericAlternate **obj, size_t size,
> +                                          Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, errp)) {
> +        return false;
> +    }
> +    /*
> +     * 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 :)

> +    return visit_start_alternate(ffv->target, name, obj, size, errp);
> +}
> +
> +static void forward_field_end_alternate(Visitor *v, void **obj)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    visit_end_alternate(ffv->target, obj);
> +}
> +
> +static bool forward_field_type_int64(Visitor *v, const char *name, int64_t *obj,
> +                                     Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, errp)) {
> +        return false;
> +    }
> +    return visit_type_int64(ffv->target, name, obj, errp);
> +}
> +
> +static bool forward_field_type_uint64(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_uint64(ffv->target, name, obj, errp);
> +}
> +
> +static bool forward_field_type_bool(Visitor *v, const char *name, bool *obj,
> +                                    Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, errp)) {
> +        return false;
> +    }
> +    return visit_type_bool(ffv->target, name, obj, errp);
> +}
> +
> +static bool forward_field_type_str(Visitor *v, const char *name, char **obj,
> +                                   Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, errp)) {
> +        return false;
> +    }
> +    return visit_type_str(ffv->target, name, obj, errp);
> +}
> +
> +static bool forward_field_type_number(Visitor *v, const char *name, double *obj,
> +                                      Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, errp)) {
> +        return false;
> +    }
> +    return visit_type_number(ffv->target, name, obj, errp);
> +}
> +
> +static bool forward_field_type_any(Visitor *v, const char *name, QObject **obj,
> +                                   Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, errp)) {
> +        return false;
> +    }
> +    return visit_type_any(ffv->target, name, obj, errp);
> +}
> +
> +static bool forward_field_type_null(Visitor *v, const char *name,
> +                                    QNull **obj, Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, errp)) {
> +        return false;
> +    }
> +    return visit_type_null(ffv->target, name, obj, errp);
> +}
> +
> +static void forward_field_optional(Visitor *v, const char *name, bool *present)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, NULL)) {
> +        *present = false;
> +        return;
> +    }
> +    visit_optional(ffv->target, name, present);
> +}
> +
> +static bool forward_field_deprecated_accept(Visitor *v, const char *name,
> +                                            Error **errp)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, errp)) {
> +        return false;
> +    }
> +    return visit_deprecated_accept(ffv->target, name, errp);
> +}
> +
> +static bool forward_field_deprecated(Visitor *v, const char *name)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    if (!forward_field_translate_name(ffv, &name, NULL)) {
> +        return false;
> +    }
> +    return visit_deprecated(ffv->target, name);
> +}
> +
> +static void forward_field_complete(Visitor *v, void *opaque)
> +{
> +    /*
> +     * Do nothing, the complete method will be called at due time
> +     * on the target visitor.
> +     */
> +}

Pattern:

* Always forward to the wrapped visitor.

* If the method takes a name, massage it with
  forward_field_translate_name() first, which can fail.

In addition, track .depth.

Loads of code, mostly boring.

> +
> +static void forward_field_free(Visitor *v)
> +{
> +    ForwardFieldVisitor *ffv = to_ffv(v);
> +
> +    g_free(ffv->from);
> +    g_free(ffv->to);
> +    g_free(ffv);
> +}
> +
> +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?

> +    v->visitor.start_struct = forward_field_start_struct;
> +    v->visitor.check_struct = forward_field_check_struct;
> +    v->visitor.end_struct = forward_field_end_struct;
> +    v->visitor.start_list = forward_field_start_list;
> +    v->visitor.next_list = forward_field_next_list;
> +    v->visitor.check_list = forward_field_check_list;
> +    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_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.complete = forward_field_complete;

This is almost in the order of visitor-impl.h.  May I have it in the
exact order?

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.

Please fix, or document as restriction; your choice.

Your tests don't cover this.  Observation, not demand.

> +
> +    v->target = target;
> +    v->from = g_strdup(from);
> +    v->to = g_strdup(to);
> +
> +    return &v->visitor;
> +}

[Tests snipped, -ENOTIME...]

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

[Paolo Bonzini]

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

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

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

Unlike the other visitors, this one isn't of a fixed type.  I think
mentioning this would be nice.  Perhaps add to the paragraph

 * There are four kinds of visitors: input visitors (QObject, string,
 * and QemuOpts) parse an external representation and build the
 * corresponding QAPI object, output visitors (QObject and string)
 * take a QAPI object and generate an external representation, the
 * dealloc visitor takes a QAPI object (possibly partially
 * constructed) and recursively frees it, and the clone visitor
 * performs a deep clone of a QAPI object.

a sentence along the lines of "The forwarding visitor is special: it
wraps another visitor, and shares its type."

>>> +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.

Since the intent is to forward *only* the alias, I wonder why we forward
*everything* when v->depth > 0.

Oh.  Is it because to get to v->depth > 0, we must have entered the
alias, so whatever we forward there must be members of the alias?

>>> +    /*
>>> +     * 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.

Yes.

> 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"?

Better.

>>> +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.

I'd prefer to restrict the forwarding visitor to the cases that make
sense and have test coverage.

>> 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.

Looks good to me apart from rather long lines in block comments.
Best to wrap these around column 70, unless the wrapping obviously
reduces legibility.

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

[Paolo Bonzini]

On 22/07/21 17:34, Markus Armbruster wrote:
>> 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).
> 
> Unlike the other visitors, this one isn't of a fixed type.  I think
> mentioning this would be nice.  Perhaps add to the paragraph

Ah okay, I didn't understand that paragraph referred to the actual 
visitors and not just the kinds in the enum.

>>> 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.
> 
> Since the intent is to forward *only* the alias, I wonder why we forward
> *everything* when v->depth > 0.
> 
> Oh.  Is it because to get to v->depth > 0, we must have entered the
> alias, so whatever we forward there must be members of the alias?

Yes, exactly.  v->depth is only nonzero after the name translation has 
succeeded (and until end_struct/end_list).

>>>> +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.
> 
> I'd prefer to restrict the forwarding visitor to the cases that make
> sense and have test coverage.

Yup, I had added an assertion in the incremental diff already.

>>> 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.
> 
> Looks good to me apart from rather long lines in block comments.
> Best to wrap these around column 70, unless the wrapping obviously
> reduces legibility.

Thanks!

paolo

[PATCH 2/2] qom: use correct field name when getting/setting alias properties

[Paolo Bonzini]

Alias targets have a different name than the alias property itself
(e.g. a machine's pflash0 might be an alias of a property named 'drive').
When the target's getter or setter invokes the visitor, it will use
a different name than what the caller expects, and the visitor will
not be able to find it (or will consume erroneously).

The solution is for alias getters and setters to wrap the incoming
visitor, and forward the sole field that the target is expecting while
renaming it appropriately.

This bug has been there forever, but it was exposed after -M parsing
switched from QemuOptions and StringInputVisitor to keyval and
QObjectInputVisitor.  Before, the visitor ignored the name. Now, it
checks "drive" against what was passed on the command line and finds
that no such property exists.

Fixes: #484
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 6a01d56546..e86cb05b84 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -20,6 +20,7 @@
 #include "qapi/string-input-visitor.h"
 #include "qapi/string-output-visitor.h"
 #include "qapi/qobject-input-visitor.h"
+#include "qapi/forward-visitor.h"
 #include "qapi/qapi-builtin-visit.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qjson.h"
@@ -2683,16 +2684,20 @@ static void property_get_alias(Object *obj, Visitor *v, const char *name,
                                void *opaque, Error **errp)
 {
     AliasProperty *prop = opaque;
+    Visitor *alias_v = visitor_forward_field(v, prop->target_name, name);
 
-    object_property_get(prop->target_obj, prop->target_name, v, errp);
+    object_property_get(prop->target_obj, prop->target_name, alias_v, errp);
+    visit_free(alias_v);
 }
 
 static void property_set_alias(Object *obj, Visitor *v, const char *name,
                                void *opaque, Error **errp)
 {
     AliasProperty *prop = opaque;
+    Visitor *alias_v = visitor_forward_field(v, prop->target_name, name);
 
-    object_property_set(prop->target_obj, prop->target_name, v, errp);
+    object_property_set(prop->target_obj, prop->target_name, alias_v, errp);
+    visit_free(alias_v);
 }
 
 static Object *property_resolve_alias(Object *obj, void *opaque,
-- 
2.31.1

Re: [PATCH 2/2] qom: use correct field name when getting/setting alias properties

[Philippe Mathieu-Daudé]

On 7/19/21 12:40 PM, Paolo Bonzini wrote:
> Alias targets have a different name than the alias property itself
> (e.g. a machine's pflash0 might be an alias of a property named 'drive').
> When the target's getter or setter invokes the visitor, it will use
> a different name than what the caller expects, and the visitor will
> not be able to find it (or will consume erroneously).
> 
> The solution is for alias getters and setters to wrap the incoming
> visitor, and forward the sole field that the target is expecting while
> renaming it appropriately.
> 
> This bug has been there forever, but it was exposed after -M parsing
> switched from QemuOptions and StringInputVisitor to keyval and
> QObjectInputVisitor.  Before, the visitor ignored the name. Now, it
> checks "drive" against what was passed on the command line and finds
> that no such property exists.
> 
> Fixes: #484

Per https://www.mail-archive.com/qemu-devel@nongnu.org/msg821579.html:

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/484

> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qom/object.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Re: [PATCH 2/2] qom: use correct field name when getting/setting alias properties

[Eric Blake]

On Mon, Jul 19, 2021 at 12:40:33PM +0200, Paolo Bonzini wrote:
> Alias targets have a different name than the alias property itself
> (e.g. a machine's pflash0 might be an alias of a property named 'drive').
> When the target's getter or setter invokes the visitor, it will use
> a different name than what the caller expects, and the visitor will
> not be able to find it (or will consume erroneously).
> 
> The solution is for alias getters and setters to wrap the incoming
> visitor, and forward the sole field that the target is expecting while
> renaming it appropriately.
> 
> This bug has been there forever, but it was exposed after -M parsing
> switched from QemuOptions and StringInputVisitor to keyval and
> QObjectInputVisitor.  Before, the visitor ignored the name. Now, it
> checks "drive" against what was passed on the command line and finds
> that no such property exists.
> 
> Fixes: #484
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qom/object.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Deceptively simple; all the work was in the previous patch writing up
the forwarding visitor.  I still wonder if Kevin's QAPI aliases will
do this more gracefully, but if we're trying to justify this as a bug
fix worthy of 6.1, this is certainly a smaller approach than Kevin's.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] qom: use correct field name when getting/setting alias properties

[Paolo Bonzini]

On 20/07/21 03:00, Eric Blake wrote:
> Deceptively simple; all the work was in the previous patch writing up
> the forwarding visitor.  I still wonder if Kevin's QAPI aliases will
> do this more gracefully, but if we're trying to justify this as a bug
> fix worthy of 6.1, this is certainly a smaller approach than Kevin's.
> 
> Reviewed-by: Eric Blake<eblake@redhat.com>

As discussed on IRC, this is unrelated to QAPI aliases; QOM alias 
properties typically target a property *on a different object*.

This is a regression, so it certainly has to be fixed in 6.1 one way or 
the other.

Paolo

Re: [PATCH 2/2] qom: use correct field name when getting/setting alias properties

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 20/07/21 03:00, Eric Blake wrote:
>> Deceptively simple; all the work was in the previous patch writing up
>> the forwarding visitor.  I still wonder if Kevin's QAPI aliases will
>> do this more gracefully, but if we're trying to justify this as a bug
>> fix worthy of 6.1, this is certainly a smaller approach than Kevin's.
>> Reviewed-by: Eric Blake<eblake@redhat.com>
>
> As discussed on IRC, this is unrelated to QAPI aliases; QOM alias
> properties typically target a property *on a different object*.

Yes, these are different beasts.

A QAPI alias provides an alternate name for a member.  The member may be
nested.  It's still within the same QAPI object.  Can be useful for
maintaining backwards compatibility, in particular for replacing (flat)
QemuOpts by QAPI-based dotted keys.

A QOM alias property is a proxy for a property of an *arbitrary* QOM
object.  Not limited to the alias's object and its sub-objects.
Strictly more powerful.

QOM alias properties are created at run time: creation requires the
target object.  QAPI aliases are completely defined at compile-time.

> This is a regression, so it certainly has to be fixed in 6.1 one way
> or the other.

Understood.

Re: [PATCH 0/2] qapi/qom: use correct field name when getting/setting alias properties

[Markus Armbruster]

Running out of time for today, so I'm sending what I have, with real
review to follow.


First, let me describe what's wrong in my own words, because that's how
I understand stuff.

Reproducer (hidden behind the link to the bug tracker):

    $ qemu-system-x86_64 \
     -blockdev '{"driver":"file","filename":"/usr/share/edk2/ovmf/OVMF_CODE.fd","node-name":"pflash0-storage","auto-read-only":true,"discard":"unmap"}' \
     -blockdev '{"node-name":"pflash0-format","read-only":true,"driver":"raw","file":"pflash0-storage"}' \
     -machine pc,pflash0=pflash0-format

The -machine gets (keyval-)parsed into a QDict, which is then processed
by object_set_properties_from_qdict() with the (keyval) QObject input
visitor.  Makes sense.

object_set_properties_from_qdict() performs a virtual struct visit
guided by the QDict's keys.  It calls object_property_set() for each
key.

For "pflash0", this calls object_property_set(obj, "pflash0", v, &err).
Since "pflash0" is a QOM alias property, this in turn calls
object_property_set(prop->target_obj, "drive0", v, &err), where prop is
the alias property, and prop->target_obj is the "cfi.pflash01" device.

Since "drive" is a drive property, this calls set_drive(), which calls
visit_type_str(v, "drive", &str, errp).  Fails, because the QDict
wrapped in visitor @v does not have a "drive" member, it has a "pflash0"
member.


Next, the solution.  I get the idea of a wrapper visitor which gives you
"pflash0" when you ask for "drive", but oh boy do I wish we could fix
the bug with a lot less code.

Re: [PATCH 0/2] qapi/qom: use correct field name when getting/setting alias properties

[Paolo Bonzini]

On 20/07/21 17:54, Markus Armbruster wrote:
> First, let me describe what's wrong in my own words, because that's how
> I understand stuff.
> 
> [snip]

All correct.

> Next, the solution.  I get the idea of a wrapper visitor which gives you
> "pflash0" when you ask for "drive", but oh boy do I wish we could fix
> the bug with a lot less code.

Yeah, if QOM didn't use visitors and just went with QObject as the 
argument to getters/setters, all this wouldn't be needed.

That said, 1/3rd of this patch is tests, and visitors do have a hidden 
advantage: they give type checking for free.  So all in all I'm not sure 
it would be better, especially now that we're starting to get more 
benefit from them (e.g. with compound properties replacing 
special-purpose command line parsing code).

Paolo

Re: [PATCH 0/2] qapi/qom: use correct field name when getting/setting alias properties

[Markus Armbruster]

Since patch submitters tend to submit code that works for the success
case, I like to test a few failure cases before anything else.  Gotcha:

    $ qemu-system-x86_64 -machine pc,pflash0=xxx qemu-system-x86_64:
    Property 'cfi.pflash01.drive' can't find value 'xxx'

The error message is misleading.

This is not a "must not commit" issue.  Fixing a regression in time for
the release at the price of a bad error message is still a win.  The bad
error message needs fixing all the same, just not necessarily before the
release.

Since mere thinking doesn't rock the release boat: any ideas on how this
could be fixed?

Re: [PATCH 0/2] qapi/qom: use correct field name when getting/setting alias properties

[Paolo Bonzini]

On 22/07/21 15:25, Markus Armbruster wrote:
> Since patch submitters tend to submit code that works for the success
> case, I like to test a few failure cases before anything else.  Gotcha:
> 
>      $ qemu-system-x86_64 -machine pc,pflash0=xxx
>      qemu-system-x86_64: Property 'cfi.pflash01.drive' can't find value 'xxx'
> 
> The error message is misleading.

Indeed I knew about this, and even thought briefly about how to fix it 
before realizing it is not a regression (which is also why I didn't 
think of including it in the commit message).

All the ways I could think about for a fix involved looking at the error 
class, and possibly even adding a dictionary of key-value pairs for some 
error classes.  I know you don't really like error classes and you 
probably would like the idea of key-value pairs even less---and to be 
honest I didn't really have a plan to implement any of that.

Paolo

> This is not a "must not commit" issue.  Fixing a regression in time for
> the release at the price of a bad error message is still a win.  The bad
> error message needs fixing all the same, just not necessarily before the
> release.
> 
> Since mere thinking doesn't rock the release boat: any ideas on how this
> could be fixed?
> 
>