Re: [Qemu-devel] [PATCH v10 13/13] qapi: Change visit_start_implicit_struct to visit_start_alternate

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> After recent changes, the only remaining use of
> visit_start_implicit_struct() is for allocating the space needed
> when visiting an alternate.  Since the term 'implicit struct' is
> hard to explain, rename the function to its current usage.  While
> at it, we can merge the functionality of visit_get_next_type()
> into the same function, making it more like visit_start_struct().
>
> Generated code is now slightly smaller:
>
> | {
> |     Error *err = NULL;
> |
> |-    visit_start_implicit_struct(v, (void**) obj, sizeof(BlockdevRef), &err);
> |+    visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
> |+                          true, &err);
> |     if (err) {
> |         goto out;
> |     }
> |-    visit_get_next_type(v, name, &(*obj)->type, true, &err);
> |-    if (err) {
> |-        goto out_obj;
> |-    }
> |     switch ((*obj)->type) {
> |     case QTYPE_QDICT:
> |         visit_type_alternate_BlockdevOptions(v, name, 
> &(*obj)->u.definition, &err);
> |         break;
> ...
> |     }
> |-out_obj:
> |-    visit_end_implicit_struct(v);
> |+    visit_end_alternate(v);
> | out:
> |     error_propagate(errp, err);
> | }
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v10: new patch
> ---
>  include/qapi/visitor.h      | 50 
> ++++++++++++++++++++++++++++++++++-----------
>  include/qapi/visitor-impl.h | 17 +++++++--------
>  scripts/qapi-visit.py       | 10 +++------
>  qapi/qapi-visit-core.c      | 40 +++++++++++++++---------------------
>  qapi/qapi-dealloc-visitor.c | 13 ++++++------
>  qapi/qmp-input-visitor.c    | 24 ++++++++--------------
>  6 files changed, 82 insertions(+), 72 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index b8ae1b5..83cad74 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -19,7 +19,6 @@
>  #include "qapi/error.h"
>  #include <stdlib.h>
>
> -
>  /* This struct is layout-compatible with all other *List structs
>   * created by the qapi generator.  It is used as a typical
>   * singly-linked list. */
> @@ -28,17 +27,52 @@ typedef struct GenericList {
>      char padding[];
>  } GenericList;
>
> +/* This struct is layout-compatible with all Alternate types
> + * created by the qapi generator. */
> +typedef struct GenericAlternate {
> +    QType type;
> +    char padding[];
> +} GenericAlternate;
> +
>  void visit_start_struct(Visitor *v, const char *name, void **obj,
>                          size_t size, Error **errp);
>  void visit_end_struct(Visitor *v, Error **errp);
> -void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
> -                                 Error **errp);
> -void visit_end_implicit_struct(Visitor *v);
>
>  void visit_start_list(Visitor *v, const char *name, Error **errp);
>  GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
>  void visit_end_list(Visitor *v);
>
> +/*
> + * Start the visit of an alternate @obj with the given @size.
> + *
> + * @name specifies the relationship to the containing struct (ignored
> + * for a top level visit, the name of the key if this alternate is
> + * part of an object, or NULL if this alternate is part of a list).
> + *
> + * @obj must not be NULL. Input visitors will allocate @obj and
> + * determine the qtype of the next thing to be visited, stored in
> + * (address@hidden)->type.  Other visitors will leave @obj unchanged.
> + *
> + * If @promote_int, treat integers as QTYPE_FLOAT.
> + *
> + * If successful, this must be paired with visit_end_alternate(), even
> + * if visiting the contents of the alternate fails.
> + */
> +void visit_start_alternate(Visitor *v, const char *name,
> +                           GenericAlternate **obj, size_t size,
> +                           bool promote_int, Error **errp);
> +
> +/*
> + * Finish visiting an alternate type.
> + *
> + * Must be called after a successful visit_start_alternate(), even if
> + * an error occurred in the meantime.
> + *
> + * TODO: Should all the visit_end_* interfaces take obj parameter, so
> + * that dealloc visitor need not track what was passed in visit_start?
> + */
> +void visit_end_alternate(Visitor *v);
> +
>  /**
>   * Check if an optional member @name of an object needs visiting.
>   * For input visitors, set address@hidden according to whether the
> @@ -47,14 +81,6 @@ void visit_end_list(Visitor *v);
>   */
>  bool visit_optional(Visitor *v, const char *name, bool *present);
>
> -/**
> - * Determine the qtype of the item @name in the current object visit.
> - * For input visitors, set address@hidden to the correct qtype of a qapi
> - * alternate type; for other visitors, leave address@hidden unchanged.
> - * If @promote_int, treat integers as QTYPE_FLOAT.
> - */
> -void visit_get_next_type(Visitor *v, const char *name, QType *type,
> -                         bool promote_int, Error **errp);
>  void visit_type_enum(Visitor *v, const char *name, int *obj,
>                       const char *const strings[], Error **errp);
>  void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error 
> **errp);
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index c4af3e0..6a1ddfb 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -22,22 +22,23 @@ struct Visitor
>                           size_t size, Error **errp);
>      void (*end_struct)(Visitor *v, Error **errp);
>
> -    void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
> -                                  Error **errp);
> -    /* May be NULL */
> -    void (*end_implicit_struct)(Visitor *v);
> -
>      void (*start_list)(Visitor *v, const char *name, Error **errp);
>      /* Must be set */
>      GenericList *(*next_list)(Visitor *v, GenericList **list, size_t size);
>      /* Must be set */
>      void (*end_list)(Visitor *v);
>
> +    /* Optional, needed for input and dealloc visitors.  */
> +    void (*start_alternate)(Visitor *v, const char *name,
> +                            GenericAlternate **obj, size_t size,
> +                            bool promote_int, Error **errp);
> +
> +    /* Optional, needed for dealloc 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);
> -    /* May be NULL; only needed for input visitors. */
> -    void (*get_next_type)(Visitor *v, const char *name, QType *type,
> -                          bool promote_int, Error **errp);
>
>      /* Must be set. */
>      void (*type_int64)(Visitor *v, const char *name, int64_t *obj,
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 02f0122..2749331 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -175,14 +175,11 @@ void visit_type_%(c_name)s(Visitor *v, const char 
> *name, %(c_name)s **obj, Error
>  {
>      Error *err = NULL;
>
> -    visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
> +    visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
> +                          %(promote_int)s, &err);
>      if (err) {
>          goto out;
>      }
> -    visit_get_next_type(v, name, &(*obj)->type, %(promote_int)s, &err);
> -    if (err) {
> -        goto out_obj;
> -    }
>      switch ((*obj)->type) {
>  ''',
>                   c_name=c_name(name), promote_int=promote_int)
> @@ -205,8 +202,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
> %(c_name)s **obj, Error
>          error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "%(name)s");
>      }
> -out_obj:
> -    visit_end_implicit_struct(v);
> +    visit_end_alternate(v);
>  out:
>      error_propagate(errp, err);
>  }
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 976106e..973ab72 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -30,21 +30,6 @@ void visit_end_struct(Visitor *v, Error **errp)
>      v->end_struct(v, errp);
>  }
>
> -void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
> -                                 Error **errp)
> -{
> -    if (v->start_implicit_struct) {
> -        v->start_implicit_struct(v, obj, size, errp);
> -    }
> -}
> -
> -void visit_end_implicit_struct(Visitor *v)
> -{
> -    if (v->end_implicit_struct) {
> -        v->end_implicit_struct(v);
> -    }
> -}
> -
>  void visit_start_list(Visitor *v, const char *name, Error **errp)
>  {
>      v->start_list(v, name, errp);
> @@ -60,6 +45,23 @@ void visit_end_list(Visitor *v)
>      v->end_list(v);
>  }
>
> +void visit_start_alternate(Visitor *v, const char *name,
> +                           GenericAlternate **obj, size_t size,
> +                           bool promote_int, Error **errp)
> +{
> +    assert(obj && size >= sizeof(GenericAlternate));
> +    if (v->start_alternate) {
> +        v->start_alternate(v, name, obj, size, promote_int, errp);
> +    }
> +}
> +
> +void visit_end_alternate(Visitor *v)
> +{
> +    if (v->end_alternate) {
> +        v->end_alternate(v);
> +    }
> +}
> +
>  bool visit_optional(Visitor *v, const char *name, bool *present)
>  {
>      if (v->optional) {
> @@ -68,14 +70,6 @@ bool visit_optional(Visitor *v, const char *name, bool 
> *present)
>      return *present;
>  }
>
> -void visit_get_next_type(Visitor *v, const char *name, QType *type,
> -                         bool promote_int, Error **errp)
> -{
> -    if (v->get_next_type) {
> -        v->get_next_type(v, name, type, promote_int, errp);
> -    }
> -}
> -
>  void visit_type_enum(Visitor *v, const char *name, int *obj,
>                       const char *const strings[], Error **errp)
>  {
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 4eae555..6922179 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -76,16 +76,15 @@ static void qapi_dealloc_end_struct(Visitor *v, Error 
> **errp)
>      }
>  }
>
> -static void qapi_dealloc_start_implicit_struct(Visitor *v,
> -                                               void **obj,
> -                                               size_t size,
> -                                               Error **errp)
> +static void qapi_dealloc_start_alternate(Visitor *v, const char *name,
> +                                         GenericAlternate **obj, size_t size,
> +                                         bool promote_int, Error **errp)
>  {
>      QapiDeallocVisitor *qov = to_qov(v);
>      qapi_dealloc_push(qov, obj);
>  }
>
> -static void qapi_dealloc_end_implicit_struct(Visitor *v)
> +static void qapi_dealloc_end_alternate(Visitor *v)
>  {
>      QapiDeallocVisitor *qov = to_qov(v);
>      void **obj = qapi_dealloc_pop(qov);
> @@ -187,8 +186,8 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
>
>      v->visitor.start_struct = qapi_dealloc_start_struct;
>      v->visitor.end_struct = qapi_dealloc_end_struct;
> -    v->visitor.start_implicit_struct = qapi_dealloc_start_implicit_struct;
> -    v->visitor.end_implicit_struct = qapi_dealloc_end_implicit_struct;
> +    v->visitor.start_alternate = qapi_dealloc_start_alternate;
> +    v->visitor.end_alternate = qapi_dealloc_end_alternate;
>      v->visitor.start_list = qapi_dealloc_start_list;
>      v->visitor.next_list = qapi_dealloc_next_list;
>      v->visitor.end_list = qapi_dealloc_end_list;
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 2621660..e659832 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -143,14 +143,6 @@ static void qmp_input_end_struct(Visitor *v, Error 
> **errp)
>      qmp_input_pop(qiv, errp);
>  }
>
> -static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
> -                                            size_t size, Error **errp)
> -{
> -    if (obj) {
> -        *obj = g_malloc0(size);
> -    }
> -}
> -
>  static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> @@ -202,19 +194,22 @@ static void qmp_input_end_list(Visitor *v)
>      qmp_input_pop(qiv, &error_abort);
>  }
>
> -static void qmp_input_get_next_type(Visitor *v, const char *name, QType 
> *type,
> -                                    bool promote_int, Error **errp)
> +static void qmp_input_start_alternate(Visitor *v, const char *name,
> +                                      GenericAlternate **obj, size_t size,
> +                                      bool promote_int, Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
>      QObject *qobj = qmp_input_get_object(qiv, name, false);
>
>      if (!qobj) {
> +        *obj = NULL;
>          error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null");
>          return;
>      }
> -    *type = qobject_type(qobj);
> -    if (promote_int && *type == QTYPE_QINT) {
> -        *type = QTYPE_QFLOAT;
> +    *obj = g_malloc0(size);
> +    (*obj)->type = qobject_type(qobj);
> +    if (promote_int && (*obj)->type == QTYPE_QINT) {
> +        (*obj)->type = QTYPE_QFLOAT;
>      }
>  }
>
> @@ -345,10 +340,10 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>
>      v->visitor.start_struct = qmp_input_start_struct;
>      v->visitor.end_struct = qmp_input_end_struct;
> -    v->visitor.start_implicit_struct = qmp_input_start_implicit_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;
> @@ -357,7 +352,6 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>      v->visitor.type_number = qmp_input_type_number;
>      v->visitor.type_any = qmp_input_type_any;
>      v->visitor.optional = qmp_input_optional;
> -    v->visitor.get_next_type = qmp_input_get_next_type;
>
>      qmp_input_push(v, obj, NULL);
>      qobject_incref(obj);

Okay, this start_alternate stuff looks like it could actually have a
chance not to confuse me every time I run across it, unlike the
implicit_struct stuff it replaces.