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

[Eric Blake <eblake@redhat.com>]

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

On 09/23/2014 08:56 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> The previous commit demonstrated that the generator overlooked some
>> fairly basic broken expressions:
>> - missing metataype
>> - metatype key has a non-string value
>> - unknown key in relation to the metatype
>> - conflicting metatype (this patch treats the second metatype as an
>> unknown key of the first key visited, which is not necessarily the
>> first key the user typed)
> 
> The whole thing is a Saturday afternoon hack, with extra hacks bolted on
> left & right.

And this series is a sequence of MY Saturday afternoon hacks in cleaning
it up :)

> 
>> Add check_keys to cover these situations, and update testcases to
>> match.  A couple other tests (enum-missing-data, indented-expr) had
>> to change since the validation added here occurs so early.
>>
>> 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 <eblake@redhat.com>
> 

>> +def check_keys(expr_elem, meta, required, optional=[]):
>> +    expr = expr_elem['expr']
>> +    info = expr_elem['info']
>> +    name = expr[meta]
> 
> Caller ensures expr[meta] exists.  Okay.
> 
>> +    if not isinstance(name, basestring):
> 
> I'm a Python noob: why basestring and not str?

Me too.  No clue.  Copy and paste from existing code.
http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/qapi.py;h=77d46aa;hb=769188d3b#l361

> 
>> +        raise QAPIExprError(info,
>> +                            "%s key must have a string value" % meta)
>> +    expr_elem['name'] = name
> 
> Where is this used?

Hmm, I know I used it at one point in my series (to be able to print the
name of an expression in check_type added in 13/19, without having to
repeat the dance of if enum: expr['enum'] elif union: expr['union'] etc.
in multiple places).  Although in my refactoring, I may have eliminated
the need for it after all.  If it's not in use at the end of the series,
I can drop it.

> 
>> +    required.append(meta)
> 
> Ugly side effect.  To avoid, either make a new list here
> 
>     required = required + [ meta ]
> 
> or do nothing here and...
> 
>> +    for (key, value) in expr.items():
>> +        if not key in required and not key in optional:
> 
> ... add "and key != meta" to this condition.

Shows my inexperience in python.  Sure, I can fix.  Looks like I get to
spin a v5.

> 
> This hunk is easier to review with whitespace ignored:

Indeed. Alas, python is a stickler about whitespace reindentation.
Since I have to respin, I'll probably split into two pieces (one with a
no-op change that reindents, the other for the improvements).


-- 
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: 539 bytes --]