Re: [Qemu-devel] [PATCH v14 02/19] qapi-visit: Add visitor.type classification

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> We have three classes of QAPI visitors: input, output, and dealloc.
> Currently, all implementations of these visitors have one thing in
> common based on their visitor type: the implementation used for the
> visit_type_enum() callback.  But since we plan to add more such
> common behavior, in relation to documenting and further refining
> the semantics, it makes more sense to have the visitor
> implementations advertise which class they belong to, so the common
> qapi-visit-core code can use that information in multiple places.
>
> For this patch, knowing the class of a visitor implementation lets
> us make input_type_enum() and output_type_enum() become static
> functions, by replacing the callback function Visitor.type_enum()
> with the simpler enum member Visitor.type.  Share a common
> assertion in qapi-visit-core as part of the refactoring.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v14: no change
> v13: no change
> v12: new patch
> ---
>  include/qapi/visitor-impl.h  | 21 ++++++++++++---------
>  qapi/qapi-visit-core.c       | 28 +++++++++++++++-------------
>  qapi/opts-visitor.c          | 12 ++----------
>  qapi/qapi-dealloc-visitor.c  |  7 +------
>  qapi/qmp-input-visitor.c     |  2 +-
>  qapi/qmp-output-visitor.c    |  2 +-
>  qapi/string-input-visitor.c  |  2 +-
>  qapi/string-output-visitor.c |  2 +-
>  8 files changed, 34 insertions(+), 42 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 2bd8f29..228a2a6 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -14,6 +14,15 @@
>
>  #include "qapi/visitor.h"
>
> +/* There are three classes of visitors; setting the class determines
> + * how QAPI enums are visited, as well as what additional restrictions
> + * can be asserted.  */
> +typedef enum VisitorType {
> +    VISITOR_INPUT,
> +    VISITOR_OUTPUT,
> +    VISITOR_DEALLOC,
> +} VisitorType;
> +
>  struct Visitor
>  {
>      /* Must be set */

I think we should explain what makes a visitor an input/output/dealloc
visitor.  Not necessarily in this patch, and not necessarily in this
place, just somewhere.  Right now, the information is scattered.

> @@ -36,10 +45,6 @@ struct Visitor
>      void (*end_alternate)(Visitor *v);
>
>      /* Must be set. */
> -    void (*type_enum)(Visitor *v, const char *name, int *obj,
> -                      const char *const strings[], Error **errp);
> -
> -    /* Must be set. */
>      void (*type_int64)(Visitor *v, const char *name, int64_t *obj,
>                         Error **errp);
>      /* Must be set. */
> @@ -58,11 +63,9 @@ struct Visitor
>
>      /* May be NULL; most useful for input visitors. */
>      void (*optional)(Visitor *v, const char *name, bool *present);
> +
> +    /* Must be set.  */
> +    VisitorType type;
>  };
>
> -void input_type_enum(Visitor *v, const char *name, int *obj,
> -                     const char *const strings[], Error **errp);
> -void output_type_enum(Visitor *v, const char *name, int *obj,
> -                      const char *const strings[], Error **errp);
> -
>  #endif
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index fa680c9..3cd7edc 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -72,12 +72,6 @@ bool visit_optional(Visitor *v, const char *name, bool *present)
>      return *present;
>  }
>
> -void visit_type_enum(Visitor *v, const char *name, int *obj,
> -                     const char *const strings[], Error **errp)
> -{
> -    v->type_enum(v, name, obj, strings, errp);
> -}
> -
>  void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp)
>  {
>      v->type_int64(v, name, obj, errp);
> @@ -208,14 +202,13 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp)
>      v->type_any(v, name, obj, errp);
>  }
>
> -void output_type_enum(Visitor *v, const char *name, int *obj,
> -                      const char *const strings[], Error **errp)
> +static void output_type_enum(Visitor *v, const char *name, int *obj,
> +                             const char *const strings[], Error **errp)
>  {
>      int i = 0;
>      int value = *obj;
>      char *enum_str;
>
> -    assert(strings);
>      while (strings[i++] != NULL);
>      if (value < 0 || value >= i - 1) {
>          error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null");
> @@ -226,15 +219,13 @@ void output_type_enum(Visitor *v, const char *name, int *obj,
>      visit_type_str(v, name, &enum_str, errp);
>  }
>
> -void input_type_enum(Visitor *v, const char *name, int *obj,
> -                     const char *const strings[], Error **errp)
> +static void input_type_enum(Visitor *v, const char *name, int *obj,
> +                            const char *const strings[], Error **errp)
>  {
>      Error *local_err = NULL;
>      int64_t value = 0;
>      char *enum_str;
>
> -    assert(strings);
> -
>      visit_type_str(v, name, &enum_str, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -257,3 +248,14 @@ void input_type_enum(Visitor *v, const char *name, int *obj,
>      g_free(enum_str);
>      *obj = value;
>  }
> +
> +void visit_type_enum(Visitor *v, const char *name, int *obj,
> +                     const char *const strings[], Error **errp)
> +{
> +    assert(strings);
> +    if (v->type == VISITOR_INPUT) {
> +        input_type_enum(v, name, obj, strings, errp);
> +    } else if (v->type == VISITOR_OUTPUT) {
> +        output_type_enum(v, name, obj, strings, errp);
> +    }
> +}
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 602f260..f98cf2e 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -507,6 +507,8 @@ opts_visitor_new(const QemuOpts *opts)
>
>      ov = g_malloc0(sizeof *ov);
>
> +    ov->visitor.type = VISITOR_INPUT;
> +
>      ov->visitor.start_struct = &opts_start_struct;
>      ov->visitor.end_struct   = &opts_end_struct;
>
> @@ -514,16 +516,6 @@ opts_visitor_new(const QemuOpts *opts)
>      ov->visitor.next_list  = &opts_next_list;
>      ov->visitor.end_list   = &opts_end_list;
>
> -    /* input_type_enum() covers both "normal" enums and union discriminators.
> -     * The union discriminator field is always generated as "type"; it should
> -     * match the "type" QemuOpt child of any QemuOpts.
> -     *
> -     * input_type_enum() will remove the looked-up key from the
> -     * "unprocessed_opts" hash even if the lookup fails, because the removal is
> -     * done earlier in opts_type_str(). This should be harmless.
> -     */
> -    ov->visitor.type_enum = &input_type_enum;
> -

Hmm, this comment doesn't look worthless.  With its statement gone, I
guess it should move somewhere else.  What do you think?

>      ov->visitor.type_int64  = &opts_type_int64;
>      ov->visitor.type_uint64 = &opts_type_uint64;
>      ov->visitor.type_size   = &opts_type_size;
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 6922179..c19a459 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -163,11 +163,6 @@ static void qapi_dealloc_type_anything(Visitor *v, const char *name,
>      }
>  }
>
> -static void qapi_dealloc_type_enum(Visitor *v, const char *name, int *obj,
> -                                   const char * const strings[], Error **errp)
> -{
> -}
> -
>  Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v)
>  {
>      return &v->visitor;
> @@ -184,6 +179,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
>
>      v = g_malloc0(sizeof(*v));
>
> +    v->visitor.type = VISITOR_DEALLOC;
>      v->visitor.start_struct = qapi_dealloc_start_struct;
>      v->visitor.end_struct = qapi_dealloc_end_struct;
>      v->visitor.start_alternate = qapi_dealloc_start_alternate;
> @@ -191,7 +187,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
>      v->visitor.start_list = qapi_dealloc_start_list;
>      v->visitor.next_list = qapi_dealloc_next_list;
>      v->visitor.end_list = qapi_dealloc_end_list;
> -    v->visitor.type_enum = qapi_dealloc_type_enum;
>      v->visitor.type_int64 = qapi_dealloc_type_int64;
>      v->visitor.type_uint64 = qapi_dealloc_type_uint64;
>      v->visitor.type_bool = qapi_dealloc_type_bool;
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 7cd1b77..02d4233 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -339,13 +339,13 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>
>      v = g_malloc0(sizeof(*v));
>
> +    v->visitor.type = VISITOR_INPUT;
>      v->visitor.start_struct = qmp_input_start_struct;
>      v->visitor.end_struct = qmp_input_end_struct;
>      v->visitor.start_list = qmp_input_start_list;
>      v->visitor.next_list = qmp_input_next_list;
>      v->visitor.end_list = qmp_input_end_list;
>      v->visitor.start_alternate = qmp_input_start_alternate;
> -    v->visitor.type_enum = input_type_enum;
>      v->visitor.type_int64 = qmp_input_type_int64;
>      v->visitor.type_uint64 = qmp_input_type_uint64;
>      v->visitor.type_bool = qmp_input_type_bool;
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index d44c676..1f2a7ba 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -234,12 +234,12 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
>
>      v = g_malloc0(sizeof(*v));
>
> +    v->visitor.type = VISITOR_OUTPUT;
>      v->visitor.start_struct = qmp_output_start_struct;
>      v->visitor.end_struct = qmp_output_end_struct;
>      v->visitor.start_list = qmp_output_start_list;
>      v->visitor.next_list = qmp_output_next_list;
>      v->visitor.end_list = qmp_output_end_list;
> -    v->visitor.type_enum = output_type_enum;
>      v->visitor.type_int64 = qmp_output_type_int64;
>      v->visitor.type_uint64 = qmp_output_type_uint64;
>      v->visitor.type_bool = qmp_output_type_bool;
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index ab12953..d604575 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -348,7 +348,7 @@ StringInputVisitor *string_input_visitor_new(const char *str)
>
>      v = g_malloc0(sizeof(*v));
>
> -    v->visitor.type_enum = input_type_enum;
> +    v->visitor.type = VISITOR_INPUT;
>      v->visitor.type_int64 = parse_type_int64;
>      v->visitor.type_uint64 = parse_type_uint64;
>      v->visitor.type_size = parse_type_size;
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index c2e5c5b..0d44d7e 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -351,7 +351,7 @@ StringOutputVisitor *string_output_visitor_new(bool human)
>
>      v->string = g_string_new(NULL);
>      v->human = human;
> -    v->visitor.type_enum = output_type_enum;
> +    v->visitor.type = VISITOR_OUTPUT;
>      v->visitor.type_int64 = print_type_int64;
>      v->visitor.type_uint64 = print_type_uint64;
>      v->visitor.type_size = print_type_size;