Re: [Qemu-devel] [PATCH v2 1/3] visitor: Add 'supported_qtypes' parameter to visit_start_alternate()

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> This will allow visitors to make decisions based on the supported qtypes
> of a given alternate type. The new parameter can replace the old
> 'promote_int' argument, as qobject-input-visitor can simply check if
> QTYPE_QINT is set in supported_qtypes.
>
> Signed-off-by: Eduardo Habkost <address@hidden>

Might conflict with Marc-André's work, which I haven't reviewed, yet.
Should be easy enough to resolve, though.

> ---
> Changes v1 -> v2:
> * Change supported_qtypes to uint32_t (Eric Blake)
> * Replace assert() on all generated visitor functions with a
>   single QEMU_BUILD_BUG_ON() on visit_start_alternate()
> * Extra spaces around "|" on generated visitor code
>   (Eric Blake)
> * Don't use bitops.h and just use (1U << QTYPE_FOO)
>   (Markus Armbruster)
> ---
>  include/qapi/visitor.h       |  5 +++--
>  include/qapi/visitor-impl.h  |  2 +-
>  scripts/qapi-visit.py        | 12 ++++++------
>  qapi/qapi-visit-core.c       |  9 ++++++---
>  qapi/qapi-clone-visitor.c    |  3 ++-
>  qapi/qapi-dealloc-visitor.c  |  3 ++-
>  qapi/qobject-input-visitor.c |  6 ++++--
>  qapi/trace-events            |  2 +-
>  8 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 1a1b62012b..8f5a223714 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -408,7 +408,8 @@ void visit_end_list(Visitor *v, void **list);
>   * 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.
> + * @supported_qtypes is a bit mask indicating which QTypes are supported
> + * by the alternate.
>   *
>   * If successful, this must be paired with visit_end_alternate() with
>   * the same @obj to clean up, even if visiting the contents of the
> @@ -416,7 +417,7 @@ void visit_end_list(Visitor *v, void **list);
>   */
>  void visit_start_alternate(Visitor *v, const char *name,
>                             GenericAlternate **obj, size_t size,
> -                           bool promote_int, Error **errp);
> +                           uint32_t supported_qtypes, Error **errp);
>  
>  /*
>   * Finish visiting an alternate type.
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index e87709db5c..8afcde0f5d 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -71,7 +71,7 @@ struct Visitor
>       * optional for output visitors. */
>      void (*start_alternate)(Visitor *v, const char *name,
>                              GenericAlternate **obj, size_t size,
> -                            bool promote_int, Error **errp);
> +                            uint32_t supported_qtypes, Error **errp);
>  
>      /* Optional, needed for dealloc visitor */
>      void (*end_alternate)(Visitor *v, void **obj);
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 5737aefa05..41c54982e2 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -161,20 +161,20 @@ void visit_type_%(c_name)s(Visitor *v, const char 
> *name, %(c_name)s *obj, Error
>  
>  
>  def gen_visit_alternate(name, variants):
> -    promote_int = 'true'
> +    qtypes = ['(1U << %s)' % (var.type.alternate_qtype())
> +              for var in variants.variants]
> +    supported_qtypes = ' | '.join(qtypes)
>      ret = ''
> -    for var in variants.variants:
> -        if var.type.alternate_qtype() == 'QTYPE_QINT':
> -            promote_int = 'false'

Note for later:

* promote_int is true if and only if we have no integer variant.

* supported_qtypes is the set of the variants' qtypes

>  
>      ret += mcgen('''
>  
>  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, 
> Error **errp)
>  {
>      Error *err = NULL;
> +    uint32_t supported_qtypes = %(supported_qtypes)s;

The initializer is of type unsigned.  I'd make the variable unsigned,
too.  Not worth a respin, though.

>  
>      visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
> -                          %(promote_int)s, &err);
> +                          supported_qtypes, &err);
>      if (err) {
>          goto out;
>      }
> @@ -183,7 +183,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
> %(c_name)s **obj, Error
>      }
>      switch ((*obj)->type) {
>  ''',
> -                 c_name=c_name(name), promote_int=promote_int)
> +                 c_name=c_name(name), supported_qtypes=supported_qtypes)
>  
>      for var in variants.variants:
>          ret += mcgen('''
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 43a09d147d..8d62914393 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -106,15 +106,18 @@ void visit_end_list(Visitor *v, void **obj)
>  
>  void visit_start_alternate(Visitor *v, const char *name,
>                             GenericAlternate **obj, size_t size,
> -                           bool promote_int, Error **errp)
> +                           uint32_t supported_qtypes,
> +                           Error **errp)
>  {
>      Error *err = NULL;
>  
> +    QEMU_BUILD_BUG_ON(QTYPE__MAX > 32);

Aha, I guess this is why you picked uint32_t.  Nevermind then.

> +
>      assert(obj && size >= sizeof(GenericAlternate));
>      assert(!(v->type & VISITOR_OUTPUT) || *obj);
> -    trace_visit_start_alternate(v, name, obj, size, promote_int);
> +    trace_visit_start_alternate(v, name, obj, size, supported_qtypes);
>      if (v->start_alternate) {
> -        v->start_alternate(v, name, obj, size, promote_int, &err);
> +        v->start_alternate(v, name, obj, size, supported_qtypes, &err);
>      }
>      if (v->type & VISITOR_INPUT) {
>          assert(v->start_alternate && !err != !*obj);
> diff --git a/qapi/qapi-clone-visitor.c b/qapi/qapi-clone-visitor.c
> index 34086cbfc0..2e40da5981 100644
> --- a/qapi/qapi-clone-visitor.c
> +++ b/qapi/qapi-clone-visitor.c
> @@ -70,7 +70,8 @@ static GenericList *qapi_clone_next_list(Visitor *v, 
> GenericList *tail,
>  
>  static void qapi_clone_start_alternate(Visitor *v, const char *name,
>                                         GenericAlternate **obj, size_t size,
> -                                       bool promote_int, Error **errp)
> +                                       uint32_t supported_qtypes,
> +                                       Error **errp)
>  {
>      qapi_clone_start_struct(v, name, (void **)obj, size, errp);
>  }
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index e39457bc79..23b64c21a4 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -38,7 +38,8 @@ static void qapi_dealloc_end_struct(Visitor *v, void **obj)
>  
>  static void qapi_dealloc_start_alternate(Visitor *v, const char *name,
>                                           GenericAlternate **obj, size_t size,
> -                                         bool promote_int, Error **errp)
> +                                         uint32_t supported_qtypes,
> +                                         Error **errp)
>  {
>  }
>  
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 865e948ac0..25997ee816 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -338,7 +338,8 @@ static void qobject_input_check_list(Visitor *v, Error 
> **errp)
>  
>  static void qobject_input_start_alternate(Visitor *v, const char *name,
>                                            GenericAlternate **obj, size_t 
> size,
> -                                          bool promote_int, Error **errp)
> +                                          uint32_t supported_qtypes,
> +                                          Error **errp)
>  {
>      QObjectInputVisitor *qiv = to_qiv(v);
>      QObject *qobj = qobject_input_get_object(qiv, name, false, errp);
> @@ -349,7 +350,8 @@ static void qobject_input_start_alternate(Visitor *v, 
> const char *name,
>      }
>      *obj = g_malloc0(size);
>      (*obj)->type = qobject_type(qobj);
> -    if (promote_int && (*obj)->type == QTYPE_QINT) {

Old condition: the alternate has no integer variant, and we got an
integer value.

> +    if (!(supported_qtypes & (1U << QTYPE_QINT)) &&
> +        (*obj)->type == QTYPE_QINT) {

New condition: same.  Good.

>          (*obj)->type = QTYPE_QFLOAT;
>      }
>  }
> diff --git a/qapi/trace-events b/qapi/trace-events
> index 339cacf0ad..b15a55b797 100644
> --- a/qapi/trace-events
> +++ b/qapi/trace-events
> @@ -11,7 +11,7 @@ visit_next_list(void *v, void *tail, size_t size) "v=%p 
> tail=%p size=%zu"
>  visit_check_list(void *v) "v=%p"
>  visit_end_list(void *v, void *obj) "v=%p obj=%p"
>  
> -visit_start_alternate(void *v, const char *name, void *obj, size_t size, 
> bool promote_int) "v=%p name=%s obj=%p size=%zu promote_int=%d"
> +visit_start_alternate(void *v, const char *name, void *obj, size_t size, 
> uint32_t supported_qtypes) "v=%p name=%s obj=%p size=%zu 
> supported_qtypes=0x%x"
>  visit_end_alternate(void *v, void *obj) "v=%p obj=%p"
>  
>  visit_optional(void *v, const char *name, bool *present) "v=%p name=%s 
> present=%p"

Reviewed-by: Markus Armbruster <address@hidden>