Re: [Qemu-devel] [PATCH v4 06/19] qapi: Better error messages for bad enums

[Eric Blake]

On 09/23/2014 08:23 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> The previous commit demonstrated that the generator had several
>> flaws with less-than-perfect enums:
>> - an enum that listed the same string twice (or two variant
>> strings that map to the same C enum) ended up generating an
>> invalid C enum
>> - because the generator adds a _MAX terminator to each enum,
>> the use of an enum member 'max' can also cause this clash
>> - if an enum omits 'data', the generator left a python stack
>> trace rather than a graceful message
>> - an enum used a non-array 'data' was silently accepted by
>> the parser
>> - an enum that used non-string members in the 'data' member
>> was silently accepted by the parser
>>
>> Add check_enum to cover these situations, and update testcases
>> to match.  While valid .json files won't trigger any of these
>> cases, we might as well be nicer to developers that make a typo
>> while trying to add new QAPI code.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---

>> --- a/tests/qapi-schema/enum-clash-member.err
>> +++ b/tests/qapi-schema/enum-clash-member.err
>> @@ -0,0 +1 @@
>> +tests/qapi-schema/enum-clash-member.json:2: enum 'MyEnum' member 'ONE' 
>> clashes with 'one'
>> diff --git a/tests/qapi-schema/enum-clash-member.exit 
>> b/tests/qapi-schema/enum-clash-member.exit
>> index 573541a..d00491f 100644
>> --- a/tests/qapi-schema/enum-clash-member.exit
>> +++ b/tests/qapi-schema/enum-clash-member.exit
>> @@ -1 +1 @@
>> -0
>> +1
>> diff --git a/tests/qapi-schema/enum-clash-member.json 
>> b/tests/qapi-schema/enum-clash-member.json
>> index cb4b428..c668ff5 100644
>> --- a/tests/qapi-schema/enum-clash-member.json
>> +++ b/tests/qapi-schema/enum-clash-member.json
>> @@ -1,2 +1,2 @@
>> -# FIXME: we should reject enums where members will clash in C switch
>> +# we reject enums where members will clash in C switch
>>  { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] }
> 
> Actually in PATCH 05 already: "clash in C switch".  I guess you mean we
> generate an enum with clashing enumeration constants.  In the test case,
> we'd generate MY_ENUM_ONE (I think) both for 'one' and 'ONE'.  Correct?

Correct; the generated C code would include an invalid switch statement
with two repetitions of the same spelling of a case label.  In patch 5,
the test case demonstrates that the parser was silently accepting code
that resulted in a clash in the generated C code; this patch updates
both qapi.py to make it a hard error, and the testsuite to change from
(accidental) pass to (intentional) error detection, so that we no longer
have to worry about the issue in the generated C code.

The same principle applies throughout my series - I first introduced new
tests in isolation for existing pre-patch behavior, with FIXME comments
where the tests expose bogus behavior, then in later patches fix the
parser to reject bogus behavior and update the test to match that it now
covers the new error message.

>> +++ b/tests/qapi-schema/enum-dict-member.err
>> @@ -0,0 +1 @@
>> +tests/qapi-schema/enum-dict-member.json:2: enum 'MyEnum' member 
>> 'OrderedDict([('value', 'str')])' is not a string
> 
> Error message is in terms of implementation instead of source.  Since
> this is merely a development tool, it'll do.  Same elsewhere, and I'm
> not going to flag it.  Precedents in master quite possible.

Yeah, I couldn't figure a way to get back at the original text typed by
the user.  I'm open to suggestions, but I'm (obviously) okay with
leaving it as is.

>> +++ b/tests/qapi-schema/enum-max-member.json
>> @@ -1,3 +1,3 @@
>> -# FIXME: we should either reject user-supplied 'max', or munge the implicit
>> -# max value we generate at the end of an array
>> +# we reject user-supplied 'max' for clashing with implicit enum end
>> +# FIXME: should we instead munge the the implicit value to avoid the clash?
> 
> Or pick an identifier for the max member so that it cannot clash with
> the ones we generate for the user's members.
> 
> The generator picks identifiers pretty much thoughtlessly in general.
> If something clashes, the C compiler spits it out, and you get to fiddle
> with the .json.

Well, hopefully by hoisting the error message away from C compilation
time (late) to qapi.py runtime (early), we have made it easier for
anyone actually needing the name 'max' to identify what still needs
fixing.  At any rate, I'm always a fan of erroring out as early as
possible, rather than waiting for an obscure crash later on in the C
compilation :)

> 
> [...]
> 
> Reviewed-by: Markus Armbruster <address@hidden>

Thanks.

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

signature.asc
Description: OpenPGP digital signature