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

[Markus Armbruster <armbru@redhat.com>]

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