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

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 26.09.2014 um 11:15 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 24.09.2014 um 09:34 hat Markus Armbruster geschrieben:
>> >> Eric Blake <eblake@redhat.com> writes:
>> >> 
>> >> > On 09/23/2014 08:56 AM, Markus Armbruster wrote:
>> >> >> Eric Blake <eblake@redhat.com> 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 <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
>> >> 
>> >> 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.
>
> As I said, the problem is probably not about the actual value of the
> string (I doubt we use anything but ASCII at the moment), but about its
> type. We may need to convert something somewhere. If you think it's
> worth the effort of finding that place, go ahead.

A value always has a concrete type.  I'd expect all string values in the
qapi scripts to be of type str.

We have to mess with Python 2's basestring only where we need to cope
with string values that may be either of type str or of type unicode.
Since there should not be any values of type unicode, I don't see why
we'd want to do the extra work required for 2.