Paolo Bonzini <pbonz...@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 <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.
>
> 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 <pbonz...@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...]
