Re: [Qemu-devel] [PATCH v2 09/19] qapi: Drop useless 'data' member of unions

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Now that we no longer have any clients of the 'void *data'
> member injected into unions, we can drop it.  Update the
> testsuite to drop the negative test union-clash-data, and
> replace it with a positive test in qapi-schema-test that
> proves that we no longer have a name collision.
>
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Daniel P. Berrange <address@hidden>
>
> ---
> v2: add R-b
> v1: drop patch that forced :empty as base to all structs
> Previously posted as part of qapi cleanup subset F:
> v6: rebase to earlier changes
> ---
>  scripts/qapi-types.py                   | 9 ---------
>  tests/Makefile                          | 1 -
>  tests/qapi-schema/qapi-schema-test.json | 2 +-
>  tests/qapi-schema/qapi-schema-test.out  | 4 ++--
>  tests/qapi-schema/union-clash-data.err  | 0
>  tests/qapi-schema/union-clash-data.exit | 1 -
>  tests/qapi-schema/union-clash-data.json | 7 -------
>  tests/qapi-schema/union-clash-data.out  | 9 ---------
>  8 files changed, 3 insertions(+), 30 deletions(-)
>  delete mode 100644 tests/qapi-schema/union-clash-data.err
>  delete mode 100644 tests/qapi-schema/union-clash-data.exit
>  delete mode 100644 tests/qapi-schema/union-clash-data.json
>  delete mode 100644 tests/qapi-schema/union-clash-data.out
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 19d1fff..0306a88 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -116,17 +116,8 @@ static inline %(base)s *qapi_%(c_name)s_base(const 
> %(c_name)s *obj)
>
>
>  def gen_variants(variants):
> -    # FIXME: What purpose does data serve, besides preventing a union that
> -    # has a branch named 'data'? We use it in qapi-visit.py to decide
> -    # whether to bypass the switch statement if visiting the discriminator
> -    # failed; but since we 0-initialize structs, and cannot tell what
> -    # branch of the union is in use if the discriminator is invalid, there
> -    # should not be any data leaks even without a data pointer.  Or, if
> -    # 'data' is merely added to guarantee we don't have an empty union,
> -    # shouldn't we enforce that at .json parse time?

I figure this comment became stale in commit 544a373.  Mention in commit
message?

>      ret = mcgen('''
>      union { /* union tag is @%(c_name)s */
> -        void *data;
>  ''',
>                  c_name=c_name(variants.tag_member.name))
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 04e34b5..cd4bbd4 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -358,7 +358,6 @@ qapi-schema += unicode-str.json
>  qapi-schema += union-base-no-discriminator.json
>  qapi-schema += union-branch-case.json
>  qapi-schema += union-clash-branches.json
> -qapi-schema += union-clash-data.json
>  qapi-schema += union-empty.json
>  qapi-schema += union-invalid-base.json
>  qapi-schema += union-optional-branch.json
> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index 632964a..b5d0c53 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -115,7 +115,7 @@
>              'number': ['number'],
>              'boolean': ['bool'],
>              'string': ['str'],
> -            'sizes': ['size'],
> +            'data': ['size'],
>              'any': ['any'] } }
>

Replaces the natural name for the size array by an arbitrary one just to
show the patch works.  Next to no value going forward, since we're no
more likely to introduce a 'data' clash than one for any number of other
names.

In short, I wouldn't bother :)

>  # testing commands
> diff --git a/tests/qapi-schema/qapi-schema-test.out 
> b/tests/qapi-schema/qapi-schema-test.out
> index f5e2a73..225e2db 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -139,9 +139,9 @@ object UserDefNativeListUnion
>      case number: :obj-numberList-wrapper
>      case boolean: :obj-boolList-wrapper
>      case string: :obj-strList-wrapper
> -    case sizes: :obj-sizeList-wrapper
> +    case data: :obj-sizeList-wrapper
>      case any: :obj-anyList-wrapper
> -enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32', 's64', 'u8', 
> 'u16', 'u32', 'u64', 'number', 'boolean', 'string', 'sizes', 'any']
> +enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32', 's64', 'u8', 
> 'u16', 'u32', 'u64', 'number', 'boolean', 'string', 'data', 'any']
>  object UserDefOne
>      base UserDefZero
>      member string: str optional=False
> diff --git a/tests/qapi-schema/union-clash-data.err 
> b/tests/qapi-schema/union-clash-data.err
> deleted file mode 100644
> index e69de29..0000000
> diff --git a/tests/qapi-schema/union-clash-data.exit 
> b/tests/qapi-schema/union-clash-data.exit
> deleted file mode 100644
> index 573541a..0000000
> --- a/tests/qapi-schema/union-clash-data.exit
> +++ /dev/null
> @@ -1 +0,0 @@
> -0
> diff --git a/tests/qapi-schema/union-clash-data.json 
> b/tests/qapi-schema/union-clash-data.json
> deleted file mode 100644
> index 7308e69..0000000
> --- a/tests/qapi-schema/union-clash-data.json
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -# Union branch 'data'
> -# FIXME: this parses, but then fails to compile due to a duplicate 'data'
> -# (one from the branch name, another as a filler to avoid an empty union).
> -# we should either detect the collision at parse time, or change the
> -# generated struct to allow this to compile.
> -{ 'union': 'TestUnion',
> -  'data': { 'data': 'int' } }
> diff --git a/tests/qapi-schema/union-clash-data.out 
> b/tests/qapi-schema/union-clash-data.out
> deleted file mode 100644
> index f5752f4..0000000
> --- a/tests/qapi-schema/union-clash-data.out
> +++ /dev/null
> @@ -1,9 +0,0 @@
> -object :empty
> -object :obj-int-wrapper
> -    member data: int optional=False
> -enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 
> 'qbool']
> -    prefix QTYPE
> -object TestUnion
> -    member type: TestUnionKind optional=False
> -    case data: :obj-int-wrapper
> -enum TestUnionKind ['data']