Re: [Qemu-devel] [PATCH] qapi-visit: Honor prefix of discriminator enum

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> When we added support for a user-specified prefix for an enum
> type (commit 351d36e), we forgot to teach the qapi-visit code
> to honor that prefix in the case of using a prefixed enum as
> the discriminator for a flat union.  While there is still some
> on-list debate on whether we want to keep prefixes, we should
> at least make it work as long as it is still part of the code
> base.
>
> Reported-by: Daniel P. Berrange <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi-visit.py                   | 3 ++-
>  tests/qapi-schema/qapi-schema-test.json | 9 ++++++---
>  tests/qapi-schema/qapi-schema-test.out  | 7 +++++--
>  3 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 0cc9b08..2bdb5a1 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -293,7 +293,8 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
> %(c_name)s **obj, Error
>      case %(case)s:
>  ''',
>                       case=c_enum_const(variants.tag_member.type.name,
> -                                       var.name))
> +                                       var.name,
> +                                       variants.tag_member.type.prefix))
>          if simple_union_type:
>              ret += mcgen('''
>          visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err);

Yup.

Let's check for completeness.  Calls of c_enum_const():

* QAPISchemaEnumType.c_null() and (with your patch) gen_visit_union()
  call it like

    c_enum_const(TYPE.name, MEMBER, TYPE.prefix)

  where MEMBER is a member of enumeration type TYPE.

  As your patch shows, the prefix is easy to forget.  A safer function
  would take just TYPE and MEMBER:

    TYPE.c_member(MEMBER)

* gen_event_send() calls

    c_enum_const(event_enum_name, name)

  where name is member of the enum type named event_enum_name.  That's
  okay because the type is auto-generated without a prefix.  Regardless,
  we could do something like

    schema.lookup_type(event_enum_name).c_member(name)

  Requires actually constructing the type, which is probably a good idea
  anyway, because it gets us the necessary collision checks.  Replacing
  global event_enum_name by event_enum_type would be nice then.  Out of
  scope for this patch.

* gen_enum_lookup() and gen_enum() work on name, values, prefix instead
  of the type.  I figure they do to support qapi-event.py.  If we clean
  it up to create the type, these functions could use the type as well,
  and then c_enum_const() could be dropped.

Bottom line for this patch: the fix is complete.

> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index 4b89527..353a34e 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -73,14 +73,17 @@
>    'base': 'UserDefZero',
>    'data': { 'string': 'str', 'enum1': 'EnumOne' } }
>
> +{ 'struct': 'UserDefUnionBase2',
> +  'base': 'UserDefZero',
> +  'data': { 'string': 'str', 'enum1': 'QEnumTwo' } }
> +
>  # this variant of UserDefFlatUnion defaults to a union that uses fields with
>  # allocated types to test corner cases in the cleanup/dealloc visitor
>  { 'union': 'UserDefFlatUnion2',
> -  'base': 'UserDefUnionBase',
> +  'base': 'UserDefUnionBase2',
>    'discriminator': 'enum1',
>    'data': { 'value1' : 'UserDefC', # intentional forward reference
> -            'value2' : 'UserDefB',
> -            'value3' : 'UserDefA' } }
> +            'value2' : 'UserDefB' } }
>
>  { 'alternate': 'UserDefAlternate',
>    'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
> diff --git a/tests/qapi-schema/qapi-schema-test.out 
> b/tests/qapi-schema/qapi-schema-test.out
> index 2c546b7..241aadb 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -121,11 +121,10 @@ object UserDefFlatUnion
>      case value2: UserDefB
>      case value3: UserDefB
>  object UserDefFlatUnion2
> -    base UserDefUnionBase
> +    base UserDefUnionBase2
>      tag enum1
>      case value1: UserDefC
>      case value2: UserDefB
> -    case value3: UserDefA
>  object UserDefNativeListUnion
>      member type: UserDefNativeListUnionKind optional=False
>      case integer: :obj-intList-wrapper
> @@ -167,6 +166,10 @@ object UserDefUnionBase
>      base UserDefZero
>      member string: str optional=False
>      member enum1: EnumOne optional=False
> +object UserDefUnionBase2
> +    base UserDefZero
> +    member string: str optional=False
> +    member enum1: QEnumTwo optional=False
>  object UserDefZero
>      member integer: int optional=False
>  event __ORG.QEMU_X-EVENT __org.qemu_x-Struct

Applied to qapi-next, thanks!