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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.

Actually, it generates a broken enum definition.

For instance,

    { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] }

produces

    typedef enum BadEnum
    {
        BAD_ENUM_ONE = 0,
        BAD_ENUM_ONE = 1,
        BAD_ENUM_MAX = 2,
    } BadEnum;

Drop "in C switch" from the comment?

>                                                             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.

That's exactly how I like it done.

>>> +++ 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.

Let's leave it as is for now.

>>> +++ 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 :)

One saturday of happy hacking, followed by years of chipping away at the
mess...

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