qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enum


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants
Date: Fri, 06 Nov 2015 11:03:52 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

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
* enum types BlkdebugEvent, BlockdevDriver, QKeyCode
* object types BlkdebugSetStateOptions, BlockDeviceInfo,
  BlockDeviceInfo, BlockDeviceStats, BlockInfo, CpuInfo, PciBusInfo,
  PciDeviceInfo, PciMemoryRegion, VncClientInfo

>>> Having two separate manglings complicates this.  Enumeration constants
>>> must be distinct after mangling with camel_to_upper().  But as soon as
>>> you use the enumeration as union tag, they must *also* be distinct
>>> after mangling with c_name().
>
> But this should already be the case - can you come up with a pair of
> valid enum values that won't clash under camel_to_upper() but would
> result in in a c_name() value collision?

One glance at camel_to_upper() should make it obvious why I'd prefer not
to have to know.  But since you asked...  I guess it can't happen,
because camel_to_upper() first mangles with c_name(), then mangles some
more.  If c_name() clashes, further mangling can't make it clash less.

>                                           The converse is not true -
> there ARE pairs of c_name() values that are distinct, but which map to
> the same mangling with camel_to_upper().

Yes.  Example: 'GotCha' and 'got-cha' both map to 'GOT_CHA'.

>                                           But if we insist that names do
> not collide case-insensitively, then that issue goes away - having
> ALL_CAP enum constants won't cause any collisions even when those
> constants are derived from member names, because member names are
> already forbidden from case-insensitive clash.
>
> There is still the question of whether we can get rid of the spurious
> collision with 'max', by putting the enum sentinel out of the namespace
> generated for other values.

I wanted to get out an RFC quickly, so I didn't try.

>                              But even with ALL_CAPS, that is possible:
>
>         BLOCK_DEVICE_IO_STATUS_OK = 0,
>         BLOCK_DEVICE_IO_STATUS_FAILED = 1,
>         BLOCK_DEVICE_IO_STATUS_NOSPACE = 2,
>         BLOCK_DEVICE_IO_STATUSMAX = 3,

Running shouted words together like STATUSMAX is even less legible than
StatusMax.  There's a reason why scriptio continua was abandoned in the
middle ages.

> Or insist that no enum value can start with a lone _ (double __ is okay
> for downstream extensions, though):
>
>         BLOCK_DEVICE_IO_STATUS_OK = 0,
>         BLOCK_DEVICE_IO_STATUS_FAILED = 1,
>         BLOCK_DEVICE_IO_STATUS_NOSPACE = 2,
>         BLOCK_DEVICE_IO_STATUS__MAX = 3,

Workable.  With the separate mangling dropped, we could do

    typedef enum BlockDeviceIoStatus {
        BlockDeviceIoStatus_ok = 0,
        BlockDeviceIoStatus_failed = 1,
        BlockDeviceIoStatus_nospace = 2,
        BlockDeviceIoStatusMAX = 3,
    } BlockDeviceIoStatus;

which I think is a least ugly solution given our convention to use
CamelCase for type names and my proposal to use the enum name as enum
constant prefix.

I'll reply to Dan separately.

[...]



reply via email to

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