[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case con
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members |
Date: |
Wed, 02 Dec 2015 09:20:33 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 11/27/2015 02:03 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>>
>>> We document that members of enums and objects should be
>>> 'lower-case', although we were not enforcing it. We have to
>>> whitelist a few pre-existing entities that violate the norms.
>>> Add three new tests to expose the new error message, each of
>>> which first uses the whitelisted name 'UuidInfo' to prove the
>>> whitelist works, then triggers the failure.
>>
>
>>> + 'X86CPURegister32',
>>
>> Because of all enum members.
>>
>> *Not* visible in QMP, thus fixable. Fix or TODO comment, please.
>
> Not visible directly in QMP, but unfortunately visible in QOM. See
> target-i386/cpu.c, where x86_reg_info_32[] references the all-caps
> names, and where x86_cpu_get_feature_words() calls
> visit_type_X86CPUFeatureWordInfoList() and thereby exposes
> X86CPURegister32 to the caller through 'qom-get'.
>
> I can certainly add comments, though.
Hmm, this shows my use of "qapi-introspect.py -u" to find the externally
visible part of QAPI is flawed.
To make it work, we'd need QOM introspection, which we might want
anyway.
Additionally, a way to output just JSON rather than JSON encoded in C
would be nice.
>>> +++ b/tests/qapi-schema/args-member-case.json
>>> @@ -0,0 +1,3 @@
>>> +# Member names should be 'lower-case' unless the struct/command is
>>> whitelisted
>>> +{ 'command': 'UuidInfo', 'data': { 'Arg': 'int' } }
>>> +{ 'command': 'Foo', 'data': { 'Arg': 'int' } }
>>
>> We normally put positive tests in qapi-schema-test.json, but I think
>> keeping this one here makes more sense.
>
> The idea of a positive test prior to a negative test, in order to prove
> that the .err file refers only to the line number of the negative test,
> was copied from returns-whitelist.json. But maybe I can reuse the
> 'no-way-this-will-get-whitelisted' name from that test :)
Yes, please.