Re: [Qemu-devel] [PATCH v9 03/17] qapi: Reserve 'u' and 'has[-_]*' member names

[Eric Blake]

On 10/19/2015 11:19 AM, Markus Armbruster wrote:

> I'm not quite comfortable with reserving 'u' now, becaue I feel we
> haven't fully explored the design space for avoiding branch - member
> clashes.
> 
> I still like the basic idea to give the unnamed union a name.  It needs
> to be a short one, to keep the C code legible.  'u' is an obvious
> option, but it requires reserving 'u' at least as member name.  '_u'
> wouldn't.  Alternatively, call the union 'u', but avoid the clash by
> mapping QAPI member name 'u' to C identifier '_u'.

Naming the union '_u' is a bit uglier (more typing in every client)
whereas munging just the member name (what about 'q_u', the way c_name
appends 'q_' to any other name that would otherwise collide) pushes the
ugliness only to the C code that actually uses a member named 'u'.

But the idea of teaching c_name() to munge 'u' as a member name
certainly seems doable, at which point we no longer need to reserve 'u'
as a member name.

> 
> I feel the decision should be made over the patch that give the union a
> name.

Well, that's patch 7/17 of this series, so at most, all I need to do is
shuffle things around when rebasing for v10.

For that matter, if we make c_name() munge 'u', it can just as easily
munge a member named 'has_' to 'q_has' (or 'base' to 'q_base' - except
that we are getting rid of that); or whatever other names we burn for
convenience on the C side.

But no one is using a member named 'u' at the moment, so it's not the
most critical problem to solve; and forbidding it is certainly
conservative (we can relax things to allow the name in QMP after all,
once we figure out the appropriate munging for the C side).


>> +++ b/scripts/qapi.py
>> @@ -488,6 +488,10 @@ def check_type(expr_info, source, value, 
>> allow_array=False,
>>      for (key, arg) in value.items():
>>          check_name(expr_info, "Member of %s" % source, key,
>>                     allow_optional=allow_optional)
>> +        if key == 'u' or key.startswith('has-') or key.startswith('has_'):
> 
> Something like c_name(key).startswith('has_') would avoid hardcoding the
> mapping of '-' to '_' here.  Dunno.

Oh, nice idea.

And looking at that, we have a number of places in qapi.py that are
using things like str[-4:] == '....' that might look nicer as
str.endwith('....'). I may add an obvious trivial cleanup patch into the
mix.


>> @@ -588,6 +592,14 @@ def check_union(expr, expr_info):
>>      # Check every branch
>>      for (key, value) in members.items():
>>          check_name(expr_info, "Member of union '%s'" % name, key)
>> +        # TODO: As long as branch names can collide with QMP names, we
>> +        # must prevent branches starting with 'has_'. However, we do not
>> +        # need to reject 'u', because that is reserved for when we start
>> +        # sticking branch names in a C union named 'u'.
>> +        if key.startswith('has-') or key.startswith('has_'):
>> +            raise QAPIExprError(expr_info,
>> +                                "Branch of union '%s' uses reserved name 
>> '%s'"
>> +                                % (name, key))
> 
> This will go away again when we give the unnamed union a name.
> 
> I feel we should punt all further clash detection until late in the
> cleanup work.  It's merely nice to have (sane error message from
> generator instead of possibly confusing one from the C compiler,
> basically), and adding it now causes churn later on.

Okay, I can respin along those lines - if my work later in the series
removes a negative test added earlier in the series, then strip that
test from the series as a whole rather than fighting the churn, to
reduce the size of the series.


>> +++ b/tests/qapi-schema/args-name-has.json
>> @@ -1,6 +1,5 @@
>>  # 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.
>> +# This would attempt to create two 'has_a' members of the C struct, one
>> +# from the flag for optional 'a', and the other from member 'has-a'.
>> +# TODO we could munge the optional flag name to avoid the collision.
> 
> You mean call them _has_FOO instead of has_FOO?  The generated code
> would be rather confusing...
> 
> If we don't want to reserve all names starting with 'has_', then I'd
> narrowly outlaw having both an optional member FOO and a member has_FOO.
> I think I'd like that a bit better than outlawing 'has_'.  But not
> enough to accept much implementation complexity.

The problem comes with child classes - we don't know a priori if an
optional member in one struct will end up being a base class to another
struct or union where the child class will hit the name clash.  It's
easier to outlaw the name, or else come up with a munging scheme that
never clashes.  Changing the existing has_ naming of flags is awkward
(lots of existing code) compared to munging the (unlikely) addition of a
new has_ member to a single qapi type.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature