qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v9 01/17] qapi: Add tests for reserved name abus


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v9 01/17] qapi: Add tests for reserved name abuse
Date: Wed, 21 Oct 2015 14:08:06 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eric Blake <address@hidden> writes:

> On 10/19/2015 10:05 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> A future patch wants to change qapi union representation from
>>> an anonymous C union over to a named C union 'u', so that the
>>> C names of tag values are in a separate namespace and thus
>>> cannot collide with the C names of non-variant QMP members.
>>> But for that to not cause any problems with future extensions
>>> to existing qapi, it would be best if we prohibit 'u' as a
>>> member name everywhere, to reserve it for our internal use.
>>> (Remember that although 'u' would only actually collide in
>>> flat unions, we must also worry about the fact that it is
>>> possible to convert from a struct to a flat union in a future
>>> qemu version while remaining backwards-compatible in QMP.)
>> 
>> This part is awkward: we're adding a negative test that fails to fail
>> for use of a name that isn't actually reserved until two patches later.
>> 
>> I guess I'd move the 'u' tests to the patch that reserves the name.  Or
>> at least start the commit message with one of the non-awkward cases :)
>
> If you are okay with it, I can squash this test into commits 2 and 3, so
> that we aren't introducing tests and changing state later, but instead
> just adding tests at the time I reserve namespace.

Sounds good.

>> I started to write that you might want to mention we already reserve the
>> 'Kind' suffix, then noticed you do in PATCH 02.  No need to say it
>> twice.
>
> Especially if I take the approach of not having a separate patch 1 that
> just adds tests.
>
> Sometimes, I find it easier to add tests showing one behavior, then flip
> the switch to show how the new behavior changes the behavior.  But I can
> be persuaded that it is just churn, and that adding the new tests at the
> same time as the new behavior is just as effective. :)

I like adding tests for currently broken behavior first, then the fix.
Your patch does that for the 'has_' prefix and the 'List' suffix.

What I don't like so much is adding non-functional tests for new
behavior first, then make the actual change.  Your patch does that for
the 'u' member name.

I'd recommend to move just the 'u' part to the patch that actually
reserves that name.

>>> On the other hand, there is no reason to forbid type name
>>> patterns from member names, or member name patterns from types,
>>> since the two are not in the same namespace in C and won't
>>> collide.
>> 
>> However, we could *choose* to enforce a single name space for
>> simplicity.  By convention, type names are StudlyCaps (except for
>> built-ins), and member names are dashed-lower-case, so collisions are
>> unlikely anyway.
>> 
>> Perhaps you should write "there is no technical reason".
>
> Well, I _do_ have a possible technical reason in mind: we've already
> mentioned that we may want to someday support automatic '-'/'_' munging;
> and it is not too much of a further jump to support case-insensitive
> matching (at least on a US keyboard, - and _ are on the same key the
> same, so it is already akin to a case conversion to go between the two).
>  With case-insensitive member names, we then have collisions between a
> MyType type name and a mytype member (where the QMP client can still
> access the member via MyType).  But yeah, I can revisit the wording of
> this paragraph.

Okay, "currently no technical reason" :)

>> Since this is a test-only patch, I'd prefix the subject with
>> "tests/qapi-schema:" instead of "qapi:".
>
> Unless I split and squash it with other patches, of course.
>
>
>>> +++ b/tests/qapi-schema/args-name-has.json
>>> @@ -0,0 +1,6 @@
>>> +# C member name collision
>>> +# FIXME - This parses, but fails to compile, because the C struct is given
>>> +# two 'has_a' members, one from the flag for optional 'a', and the other
>>> +# from member 'has-a'.  Either reject this at parse time, or munge the C
>>> +# names to avoid the collision.
>>> +{ 'command': 'oops', 'data': { '*a': 'str', 'has-a': 'str' } }
>> 
>> Complements existing args-name-clash.json, which tests 'a-b' and 'a_b'.
>> 
>> Call it args-has-clash.json?
>
> Sure, can do.
>
>>> +++ b/tests/qapi-schema/flat-union-clash-branch.json
>>> @@ -1,18 +1,15 @@
>>>  # Flat union branch name collision
>>> -# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
>>> -# (one from the base member, the other from the branch name).  We should
>>> -# either reject the collision at parse time, or munge the generated branch
>>> -# name to allow this to compile.
>>> +# FIXME: this parses, but then fails to compile due to a duplicate 'has_a'
>>> +# (one as the implicit flag for the optional base member, the other from
>>> +# the C member for the branch name).  We should either reject the collision
>>> +# at parse time, or munge the generated branch name to allow this to 
>>> compile.
>>>  { 'enum': 'TestEnum',
>>> -  'data': [ 'base', 'c-d' ] }
>>> +  'data': [ 'has-a' ] }
>>>  { 'struct': 'Base',
>>> -  'data': { 'enum1': 'TestEnum', '*c_d': 'str' } }
>>> +  'data': { 'enum1': 'TestEnum', '*a': 'str' } }
>>>  { 'struct': 'Branch1',
>>>    'data': { 'string': 'str' } }
>>> -{ 'struct': 'Branch2',
>>> -  'data': { 'value': 'int' } }
>>>  { 'union': 'TestUnion',
>>>    'base': 'Base',
>>>    'discriminator': 'enum1',
>>> -  'data': { 'base': 'Branch1',
>>> -            'c-d': 'Branch2' } }
>>> +  'data': { 'has-a': 'Branch1' } }
>> 
>> This replaces the test of branch name 'c-d' clashing with member 'c_d'
>> by a test of branch name 'has-a' clashing with the has_a flag of
>> optional member 'a'.  Okay, since flat-union-clash-type.json covers
>> clash of branch name with member name.
>
> And since the test disappears completely later in my pending work, once
> I change the layout to use a named union.  (See subset C 6/14
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg04111.html)
>
>>> +# Even if 'u' is forbidden as a struct member name, it should still be
>>> +# valid as a type or union branch name. And although '*Kind' is forbidden
>>> +# as a type name, it should not be forbidden as a member or branch name.
>>> +# TODO - '*List' should also be forbidden as a type name, and 'has_*' as
>>> +# a member name.
>>> +{ 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'] } }
>>> +{ 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a',
>>> +                          'myList': 'has_a' } }
>>> +
>>>  # testing commands
>>>  { 'command': 'user_def_cmd', 'data': {} }
>>>  { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
>> 
>> Value of these positive tests seems marginal.  But if you think they're
>> worth keeping, I'll take them.
>
> I added even more of these positive tests in subset C.  And then I
> noticed that you've made the same comment about the positive test I
> added in patch 4/17.  I can go either way (the added positive tests
> helped me ensure that things compiled while writing patches, but I won't
> be offended to omit them if you don't think they add enough to keep
> long-term).

Let's drop the positive tests that are really just covering special
cases that soon will be history.

>>> +++ b/tests/qapi-schema/struct-member-u.json
>>> @@ -0,0 +1,6 @@
>>> +# Potential C member name collision
>>> +# FIXME - This parses and compiles, but would cause a collision if this
>>> +# struct is later reworked to be part of a flat union, once unions hide
>>> +# their tag values under a C union named 'u'. We should reject the use
>>> +# of this identifier to reserve it for internal use.
>>> +{ 'struct': 'Oops', 'data': { 'u': 'str' } }
>> 
>> If the later patch outlaws 'u' in structs as well, this test will do,
>> only the comment will change.
>> 
>> If it outlaws 'u' only where it actually clashes, namely in unions, the
>> test will need updating.
>> 
>> More reason to move the test to the patch that does the outlawing.
>
> Fair enough. And yes, I outlawed 'u' everywhere, not just in unions, on
> the grounds that it is possible to convert a struct to a union while
> remaining backwards-compatible in the QMP that it accepts; the act of
> converting between object types must not invalidate an existing object
> member, so if we reserve a member name, we should reserve it for all
> objects and not just qapi unions.

An argument could be made either way, but it's best made in the context
of the patch that actually does the outlawing :)



reply via email to

[Prev in Thread] Current Thread [Next in Thread]