Re: [Qemu-devel] [PATCH for-2.9 06/47] qapi: Have each QAPI schema declare its name rule violations

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3031 bytes --]

On 03/13/2017 01:18 AM, Markus Armbruster wrote:
> qapi.py has a hardcoded white-list of type names that may violate the
> rule on use of upper and lower case.  Add a new pragma directive
> 'name-case-whitelist', and use it to replace the hard-coded
> white-list.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qapi-code-gen.txt                  |  6 ++++++
>  qapi-schema.json                        | 11 ++++++++++-
>  scripts/qapi.py                         | 22 ++++++++++------------
>  tests/qapi-schema/enum-member-case.err  |  2 +-
>  tests/qapi-schema/enum-member-case.json |  1 +
>  5 files changed, 28 insertions(+), 14 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -61,7 +61,16 @@
>          'query-migrate-cache-size',
>          'query-tpm-models',
>          'query-tpm-types',
> -        'ringbuf-read' ] } }
> +        'ringbuf-read' ],
> +    'name-case-whitelist': [
> +        'ACPISlotType',         # DIMM, visible through query-acpi-ospm-status
> +        'CpuInfoMIPS',          # PC, visible through query-cpu
> +        'CpuInfoTricore',       # PC, visible through query-cpu
> +        'QapiErrorClass',       # all members, visible through errors
> +        'UuidInfo',             # UUID, visible through query-uuid
> +        'X86CPURegister32',     # all members, visible indirectly through qom-get
> +        'q_obj_CpuInfo-base'    # CPU, visible through query-cpu
> +    ] } }

Interesting - here you bunch up 2 of the 3 pragmas into one dict, while
still leaving the third related to documentation in its own dict.

That 'q_obj_CpuInfo-base' is ugly, and I had a patch around previously
that used a saner name rather than making callers reverse-engineer the
implicit naming rules.  Related to my work on anonymous bases to flat
unions, so I'll get to rebase that work and post it on top of yours.
But not a show-stopper for this patch, where it is just moving the location.


> @@ -311,6 +302,13 @@ class QAPISchemaParser(object):
>                                     "Pragma returns-whitelist must be"
>                                     " a list of strings")
>              returns_whitelist = value
> +        elif name == 'name-case-whitelist':
> +            if (not isinstance(value, list)
> +                    or any([not isinstance(elt, str) for elt in value])):
> +                raise QAPISemError(info,
> +                                   "Pragma name-case-whitelist must be"
> +                                   " a list of strings")
> +            name_case_whitelist = value

Same comments as before - new error message with no testsuite coverage,
and no checking for duplicate assignment where last one silently wins;
but where I'm okay with deferring if you don't want to delay 2.9 for a
v2 respin.

So if you use it unchanged,
Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]