Re: [Qemu-devel] [PATCH v3 02/10] qapi: allow QmpInputVisitor to auto-cast types

[Eric Blake]

On 03/22/2016 09:49 AM, Daniel P. Berrange wrote:
> On Mon, Mar 21, 2016 at 05:18:01PM -0600, Eric Blake wrote:
>> On 03/10/2016 11:59 AM, Daniel P. Berrange wrote:
>>> Currently the QmpInputVisitor assumes that all scalar
>>> values are directly represented as their final types.
>>> ie it assumes an 'int' is using QInt, and a 'bool' is
>>> using QBool.
>>>
>>> This extends it so that QString is optionally permitted
>>> for any of the non-string scalar types. This behaviour
>>> is turned on by requesting the 'autocast' flag in the
>>> constructor.
>>>
>> Hmm.  Do we need to worry about partial asymmetry?  That is,
>> qint_get_int() returns a signed number, but qemu_strtoull() parses
>> unsigned; if the original conversion from JSON to qint uses a different
>> parser, then we could have funny results where we get different results
>> for things like:
>>  "key1":9223372036854775807, "key2":"9223372036854775807",
>> even though the same string of digits is being parsed, based on whether
>> the different parsers handle numbers larger than INT64_MAX differently.
> 
> Is this something you want me to change for re-post, or just a general
> point for future ?  ie should I be using qemu_strtoll instead of
> qemu_strtoull or something else ?   qint itself doesn't seem
> to concern itself with parsing ints from strintgs, so presumably
> this is from json code ?

General comment for now. We already know we need a bigger audit of
handling of values larger than INT64_MAX, so any cleanups related to
that can be deferred to that later audit.  But maybe a FIXME or TODO
comment in the code in your submission, to remind us to think about it
during the later audit, would help.


>>> +    qstr = qobject_to_qstring(qobj);
>>> +    if (qstr && qstr->string && qiv->autocast) {
>>> +        if (!strcasecmp(qstr->string, "on") ||
>>> +            !strcasecmp(qstr->string, "yes") ||
>>> +            !strcasecmp(qstr->string, "true")) {
>>> +            *obj = true;
>>> +            return;
>>> +        }
>>
>> Do we also want to allow "0"/"1" for true/false?
> 
> These 3 strings I took from opts-visitor.c, so to maintain compat
> we probably should not allow 0/1, unless we decide to extend
> opts-visitor too

Good point.  Maybe a comment pointing in both places pointing out that
we should keep them in sync is a worthwhile addition.

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

signature.asc
Description: OpenPGP digital signature