Re: [Qemu-devel] [PATCH v12 27/36] qapi: Forbid case-insensitive clashes

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 11/19/2015 09:50 AM, Markus Armbruster wrote:
>> Let's think through this on a higher level.
>> 
>> I figure the motivation for this patch is twofold:
>> 
>> 1. C identifier clash detection
>> 
>
>> 
>> 2. Dislike for interfaces that differ only in case
>
> And the related dislike for interfaces that differ only in '-' or '_'.

Yes.

>> Our naming conventions actually make clashes due to folding relatively
>> unlikely.  They are:
>> 
>> * All names consist of letters, digits, '-' and '_', starting with a
>>   letter.
>> 
>> * Event names are ALL_CAPS, i.e. they use neither lower case nor '-'.
>> 
>>   Without lower case, '-' and '.', clash due to folding is impossible.
>
> So if we enforce that convention, we don't have to worry about case
> clash, and I don't think we have any outliers to whitelist.

qmp-introspect.c shows none.

>> * Command names, member names and built-in type names are dashed-lower,
>>   i.e. they use neither upper case nor '_'.
>> 
>>   Without upper case, '_', and '.', clash due to folding is impossible.
>
> If we enforce that convention, we have to whitelist outliers (for
> example, 'netdev_add' as a command name, or a number of types whose
> members still use '_').

Yes.

>                          The existence of a whitelist will discourage
> future clashes, without any of the hassle of coding up clash checks.

I hope so.

>> * Type names are CamelCase.  They do not use '_' or '-'.
>> 
>>   Can theoretically clash in amusing ways: ONegus, OneGus.
>>   Ain'tCamelCaseFun!
>
> Do we have any outliers?

Let's look.  Stick

            print '%s %s' % (entity.__class__.__name__, name)

into QAPISchema.visit()'s loop.

Pipe output through "sed -n 's/^.*Type \([^:].*\)$/\1/p'" to get the
type names.  Grep for '[-_]', -v '[a-z]' and -v '[A-Z]' shows there are
no names with [-_], no names without lower case letters, and the only
names without upper case letters are the 14 built-in types.  Both for
qapi-schema.json and qga/qapi-schema.json.

>>   We can get rid of the clashes by not case folding type names.  See
>>   "[PATCH RFC 3/5] qapi: Use common name mangling for enumeration
>>   constants".  The patch additionally drops folding of enumeration
>>   member names, which isn't necessary.  Controversial.
>
> There is even the possibility of mixed treatment (enumeration name
> portion as-is, member name portion shouted, as in 'MyTypeVALUE1').  Not
> sure if we'll get a clear answer to the controversy, but also not sure
> it is worth holding up other patches while discussing that.

I'm content to shout the member name.  I'd stick a '_' between the
prefix and the member name, though.

>> * Additionally, any name may be prefixed by __RFQDN_.  RFQDN consists of
>>   letters, digits '-' and '.'.
>> 
>>   Because unprefixed names start with a letter, and the prefix starts
>>   with '__', prefixed names cannot clash with unprefixed names.
>> 
>>   If we additionally stipulated that event prefixes are upper case, and
>>   all others lower case, prefixes couldn't contribute to clashes at all.
>
> It's a bit weird that we'd have __org.qemu_command and __ORG.QEMU_EVENT
> for the same vendor.

Yes.

>                       On the other hand, FQDN is already
> case-insensitive (qemu.org and QEMU.ORG resolve to the same address).

Yes, and that makes stupilating case now somewhat problematic.

> So there won't be clashes between vendors (as no two vendors can share a
> FQDN that differs in case); beyond that, if a vendor wants to play weird
> case games, that's their (downstream) problem, not ours.

Yes, not our worry.

>> Strict adherence to our naming conventions would eliminate clashes due
>> to folding except for type names.  A single extra dictionary mapping
>> c_name(typ.name).lower() to typ would suffice.
>
> Certainly may be simpler than carrying three dictionaries for
> command/event/type.
>
>> 
>> What happens to the rest of your queue if we shelve this patch for now?
>
> Not much; in fact, according to the patch version log:
>
> ---
> v12: add in entity case collisions (required sharing two maps),
> improve commit message
> v11: rebase to latest, don't focus so hard on future case-insensitive
> extensions, adjust commit message
> v10: new patch
> ---
>
> I only even added it due to conversations on v9, and intentionally kept
> it separate from 'qapi: Detect collisions in C member names' (we
> absolutely want to report exact-name collisions, whether or not we also
> decide to report case collisions).  It should not be a problem to defer
> this patch (or a better version of it that enforces conventions, adds
> whitelist handling, then only worries about type name case collisions)
> to much later, if at all.

Okay, let's shelve the patch for now, keep the queue moving, and revisit
the patch at a more opportune time.

I hope the detour didn't eat too much of your time.  We learned a few
things, and perhaps we can still reuse some of the work.