[Qemu-devel] What to do about QAPI naming convention violations (was: [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants)

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Markus Armbruster <address@hidden> writes:
>
>> Eric Blake <address@hidden> writes:
>>
>>> On 11/05/2015 09:01 AM, Daniel P. Berrange wrote:
>>>> On Thu, Nov 05, 2015 at 04:30:00PM +0100, Markus Armbruster wrote:
>>>>> QAPI names needn't be valid C identifiers, so we mangle them with
>>>>> c_name().  Except for enumeration constants, which we mangle with
>>>>> camel_to_upper().
>>>>>
>>>>> c_name() is easy enough to understand: replace '.' and '-' by '_',
>>>>> prefix certain ticklish identifiers with 'q_'.
>>>>>
>>>>> camel_to_upper() is a hairball of heuristics, and guessing how it'll
>>>>> mangle interesting input could serve as a (nerdy) game.  Despite some
>>>>> tweaking (commit 5d371f4), it's still inadqeuate for some QAPI names
>>>>> (commit 351d36e).
>>>
>>> One of the issues at hand is whether we want to (eventually) teach QMP
>>> to be case-insensitive.  Right now, our c_name() mangling preserves case
>>> (you can have a struct with members 'a' and 'A'), although (hopefully)
>>> no one is relying on it.  But camel_to_upper() is case-insensitive ('a'
>>> and 'A' would result in the same enum constant).
>>>
>>> In order to (later) support case-insensitive QMP, we need to decide up
>>> front that we will not allow any qapi member names to collide
>>> case-insensitively (outlaw 'a' and 'A' in the same struct; although the
>>> C code is still case-preserving); and now that this series is adding a
>>> single check_clash() function, it's very easy to do.  In fact, I'll add
>>> that to my series for 2.5 (it's always easier to reserve something now,
>>> especially if no one was using it, and then relax later; than it is to
>>> try and restrict things later but run into counter-cases).
>>
>> I doubt QMP should be made case-insensitive.  JSON isn't, C isn't.  Our
>> use of case is actually fairly consistent: event names are ALL_CAPS,
>> everything else is in lower case.  Complete list of exceptions found in
>> result of query-qmp-schema:
>>
>> * struct UuidInfo member UUID
>> * struct CpuInfo members CPU and PC
>> * enum ACPISlotType member DIMM
>> * enum InputButton members Left, Middle, Right, WheelUp, WheelDown
>> * enum InputAxis members X, Y
>>
>> That said, an interface where names differ only in case is a badly
>> designed interface.  I'd be fine with rejecting such abuse.
>>
>> Oddballs not related to case:
>>
>> * enum BlkdebugEvent uses '.' in member names
>> * enum QKeyCode uses member names starting with a digit
>>
>> For me, the one argument for some kind of insensitivity is our idiotic
>> habit to sometimes string words together with '_' instead of '-', which
>> has led to an unholy mess.  The offenders are
>>
>> * commands block_passwd, block_resize, block_set_io_throttle,
>>   client_migrate_info, device_del, expire_password, migrate_cancel,
>>   migrate_set_downtime, migrate_set_speed, netdev_add, netdev_del,
>>   set_link, set_password, system_powerdown, system_reset, system_wakeup

Missing: add_client.

>> * enum types BlkdebugEvent, BlockdevDriver, QKeyCode
>> * object types BlkdebugSetStateOptions, BlockDeviceInfo,
>>   BlockDeviceInfo, BlockDeviceStats, BlockInfo, CpuInfo, PciBusInfo,
>>   PciDeviceInfo, PciMemoryRegion, VncClientInfo
>
> I can think of a few ways to clean up the '_' vs. '-' mess:
>
> 1. Fix the offenders, keep the unfixed names as aliases.
>
>    Requires an alias mechanism.
>
>    If we do it in 2.5, we can keep the aliases out of QMP introspection.
>
> 2. Fix the offenders, map '_' to '-' in QMP input, but only in object
>    keys and values of enumeration type, not other strings.
>
>    Distinguishing the two kinds of strings might be non-trivial, dunno.
>
> 3. Compare '_' and '-' equal in all the necessary places.
>
>    Need to find these places.
>
>    The mess remains visible in QMP introspection unless we also fix the
>    offenders.

Of course, there's a big difference between QMP input and output.

On input, we can accept a nicer name in addition to the ugly name, and
that's compatible.

On output, we can only duplicate data with an ugly name under a nicer
one.  Duplicating members or events doesn't feel like an improvement.
That leaves deprecating commands.

I had a closer look at how the screwy names are used in QMP to see how
much of the mess is fixable within reason.

Command names are all fixable.

InputButton and InputAxis are input for x-input-send-event.  Fixable.
May not even need backward compatibility.

QKeyCode is input for x-input-send-event and send-key.  Fixable.
However, I think we want to bend the rules instead.

ACPISlotType, BlockDeviceInfo, BlockDeviceStats, BlockInfo, CpuInfo,
PciBusInfo, PciDeviceInfo, PciMemoryRegion, UuidInfo, VncClientInfo are
all output of some query command.  The only fix is deprecating the query
commands, which is a big hammer.

BlkdebugEvent is related to the external blkdebug interface described by
docs/blkdebug.txt.  The two are actually decoupled, i.e. changing the
QAPI names doesn't affect the external debugging interface, and vice
versa.  Making them different can only cause confusion, though.  Not
sure we can fix the names.

BlkdebugSetStateOptions and BlockdevDriver are input for blockdev-add.
Fixable.  May not even need backward compatibility.

> Fixing the offenders without keeping the unfixed names along changes QMP
> introspection value incompatibly.  May not be practical after 2.5.
>
> I wish I had made the connection between the '_' vs. '-' mess and
> introspection earlier.
>
> If we decide to clean up '_' vs. '-', then other irregularities like
> case could be cleaned up along with it.
>
> Thoughts?
>
> [...]

Here's what I think we should do:

* Make qapi.py enforce the naming conventions, to stop us from digging
  ourselves deeper into this hole.

  Requires a whitelist, similar to returns_whitelist.  I'd whitelist
  whole types, not members.

* Try to fix x-input-send-event and blockdev-add before they become
  unfixable.

* Live with the rest.