Re: [Qemu-devel] [PATCH v2 2/3] qapi: Add enum_table[] parameter to start_alternate

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> The new parameter will be used by the string input visitor to detect
> alternate types that can't be parsed unambiguously.
>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> Changes v1 -> v2:
> * (new patch added to series)
> ---
>  include/qapi/visitor.h       | 10 +++++++++-
>  include/qapi/visitor-impl.h  |  4 +++-
>  scripts/qapi-visit.py        | 11 +++++++++--
>  qapi/qapi-visit-core.c       |  7 +++++--
>  qapi/qapi-clone-visitor.c    |  1 +
>  qapi/qapi-dealloc-visitor.c  |  1 +
>  qapi/qobject-input-visitor.c |  1 +
>  qapi/trace-events            |  2 +-
>  8 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 8f5a223714..05e5ca0eb2 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -411,13 +411,21 @@ void visit_end_list(Visitor *v, void **list);
>   * @supported_qtypes is a bit mask indicating which QTypes are supported
>   * by the alternate.
>   *
> + * @enum_table contains the enum value lookup table, in case
> + * strings in the input are going to be parsed as enums. Visitors

I agree with Eric: spelling out it's null otherwise wouldn't hurt.

Also: please indulge me and put two spaces after sentence-ending
punctuation.

> + * aren't required to validate string input according to
> + * enum_table, as visit_type_enum() will be called automatically
> + * if (*obj)->type is QTYPE_QSTRING.
> + *
>   * If successful, this must be paired with visit_end_alternate() with
>   * the same @obj to clean up, even if visiting the contents of the
>   * alternate fails.
>   */
>  void visit_start_alternate(Visitor *v, const char *name,
>                             GenericAlternate **obj, size_t size,
> -                           uint32_t supported_qtypes, Error **errp);
> +                           uint32_t supported_qtypes,
> +                           const char *const enum_table[],
> +                           Error **errp);
>  
>  /*
>   * Finish visiting an alternate type.
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 8afcde0f5d..b98370fabb 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -71,7 +71,9 @@ struct Visitor
>       * optional for output visitors. */
>      void (*start_alternate)(Visitor *v, const char *name,
>                              GenericAlternate **obj, size_t size,
> -                            uint32_t supported_qtypes, Error **errp);
> +                            uint32_t supported_qtypes,
> +                            const char *const enum_table[],
> +                            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 41c54982e2..2f4dc56918 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -166,6 +166,12 @@ def gen_visit_alternate(name, variants):
       qtypes = ['(1U << %s)' % (var.type.alternate_qtype())
                 for var in variants.variants]
>      supported_qtypes = ' | '.join(qtypes)
>      ret = ''
>  
> +    enum_table = 'NULL'
> +    for var in variants.variants:
> +        if isinstance(var.type, QAPISchemaEnumType):
> +            enum_table = '%s_lookup' % (var.type.c_name())
> +            break
> +
>      ret += mcgen('''
>  

Iterating over variants.variants again is less than elegant, but I don't
have better ideas.

>  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, 
> Error **errp)
> @@ -174,7 +180,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
> %(c_name)s **obj, Error
>      uint32_t supported_qtypes = %(supported_qtypes)s;
>  
>      visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
> -                          supported_qtypes, &err);
> +                          supported_qtypes, %(enum_table)s, &err);
>      if (err) {
>          goto out;
>      }
> @@ -183,7 +189,8 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
> %(c_name)s **obj, Error
>      }
>      switch ((*obj)->type) {
>  ''',
> -                 c_name=c_name(name), supported_qtypes=supported_qtypes)
> +                 c_name=c_name(name), supported_qtypes=supported_qtypes,
> +                 enum_table=enum_table)
>  
>      for var in variants.variants:
>          ret += mcgen('''
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 8d62914393..479aa763c8 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -107,6 +107,7 @@ void visit_end_list(Visitor *v, void **obj)
>  void visit_start_alternate(Visitor *v, const char *name,
>                             GenericAlternate **obj, size_t size,
>                             uint32_t supported_qtypes,
> +                           const char *const enum_table[],
>                             Error **errp)
>  {
>      Error *err = NULL;
> @@ -115,9 +116,11 @@ void visit_start_alternate(Visitor *v, const char *name,
>  
>      assert(obj && size >= sizeof(GenericAlternate));
>      assert(!(v->type & VISITOR_OUTPUT) || *obj);
> -    trace_visit_start_alternate(v, name, obj, size, supported_qtypes);
> +    trace_visit_start_alternate(v, name, obj, size, supported_qtypes,
> +                                (void *)enum_table);
>      if (v->start_alternate) {
> -        v->start_alternate(v, name, obj, size, supported_qtypes, &err);
> +        v->start_alternate(v, name, obj, size, supported_qtypes,
> +                           enum_table, &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 2e40da5981..9340441ee6 100644
> --- a/qapi/qapi-clone-visitor.c
> +++ b/qapi/qapi-clone-visitor.c
> @@ -71,6 +71,7 @@ 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,
>                                         uint32_t supported_qtypes,
> +                                       const char *const enum_table[],
>                                         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 23b64c21a4..fed366c660 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -39,6 +39,7 @@ 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,
>                                           uint32_t supported_qtypes,
> +                                         const char *const enum_table[],
>                                           Error **errp)
>  {
>  }
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 25997ee816..ae4a6a04e1 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -339,6 +339,7 @@ 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,
>                                            uint32_t supported_qtypes,
> +                                          const char *const enum_table[],
>                                            Error **errp)
>  {
>      QObjectInputVisitor *qiv = to_qiv(v);
> diff --git a/qapi/trace-events b/qapi/trace-events
> index b15a55b797..384c251814 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, 
> uint32_t supported_qtypes) "v=%p name=%s obj=%p size=%zu 
> supported_qtypes=0x%x"
> +visit_start_alternate(void *v, const char *name, void *obj, size_t size, 
> uint32_t supported_qtypes, void *enum_table) "v=%p name=%s obj=%p size=%zu 
> supported_qtypes=0x%x enum_table=%p"
>  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"

Not yet sure we need this, but if we do, clarify the function comment,
and you may add

Reviewed-by: Markus Armbruster <address@hidden>