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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 24.09.2014 um 09:34 hat Markus Armbruster geschrieben:
>> Eric Blake <address@hidden> writes:
>> 
>> > On 09/23/2014 08:56 AM, Markus Armbruster wrote:
>> >> Eric Blake <address@hidden> writes:
>> >>> 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 <address@hidden>
>> >> 
>> >
>> >>> +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
>> 
>> Yes.  It's our only use of basestring.  Other places use isinstance(FOO,
>> str).
>> 
>> The basestring use comes from Kevin's commit b35284e.  Kevin, why
>> basestring and not str?
>
> Do I look as if I knew what I'm doing when I write Python code? :-)
>
> Apparently basestring is the superclass for ASCII and Unicode strings. I
> seem to dimly remember that I did indeed get a Unicode string somewhere
> (even though probably no non-ASCII characters in it) and it caused
> trouble. Might well have been here.

I think there are two sane ways forward:

1. Declare QAPI schemas to be ASCII only for now.  Replace the one
instance of basestring by plain str.

2. Fix the code to work with both ASCII and Unicode strings even in
Python 2.  Package six could be used.

I'm voting for 1.