From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44146) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuhB9-00068P-Hv for qemu-devel@nongnu.org; Fri, 06 Nov 2015 08:35:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZuhB4-0006ih-Be for qemu-devel@nongnu.org; Fri, 06 Nov 2015 08:35:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39126) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuhB3-0006iT-JP for qemu-devel@nongnu.org; Fri, 06 Nov 2015 08:35:38 -0500 From: Markus Armbruster References: <1446618049-13596-22-git-send-email-eblake@redhat.com> <1446737402-15597-1-git-send-email-armbru@redhat.com> <1446737402-15597-4-git-send-email-armbru@redhat.com> <20151105160114.GG15525@redhat.com> <563B86CD.4090203@redhat.com> <87a8qrculz.fsf@blackfin.pond.sub.org> Date: Fri, 06 Nov 2015 14:35:33 +0100 In-Reply-To: <87a8qrculz.fsf@blackfin.pond.sub.org> (Markus Armbruster's message of "Fri, 06 Nov 2015 11:03:52 +0100") Message-ID: <87mvur5jyy.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Markus Armbruster writes: > Eric Blake 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 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. 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? [...]