qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v3 02/10] qapi: allow QmpInputVisitor to auto-cast types
Date: Tue, 22 Mar 2016 10:20:16 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]