On 05/09/2013 08:20 PM, Michael Roth wrote: > Currently our JSON parser assumes that numbers lacking a mantissa are > integers and attempts to store them as QInt/int64 values. This breaks in > the case where the number overflows/underflows int64 values (which is > still valid JSON) > > Fix this by detecting such cases and using a QFloat to store the value > instead. > > Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com> > --- > qobject/json-parser.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-)
This changes the error message handed back to QMP clients, and possibly exposes problems in other qemu code that receives the result of json parses. Previously, for an 'int' argument, if you passed in a too-large number, you got an error that the argument was too large for int. Now, the number is accepted as a double; are we guaranteed that in a context that expects a qint, when that code is now handed a qfloat (a case which was previously impossible because qint_from_int protected it), that the code will still behave correctly? At any rate, libvirt already checks that all numbers that fall outside the range of int64_t are never passed over qmp when passing an int argument (and yes, this is annoying, in that large 64-bit unsigned numbers have to be passed as negative numbers, rather than exceeding INT64_MAX), so libvirt should not be triggering this newly exposed code path. But even if libvirt doesn't plan on triggering it, I'd still feel better if your commit message documented evidence of testing what happens in this case. For example, compare what {"execute":"add-fd","arguments":{"fdset-id":"99999999999999999999"}} does before and after this patch. > + /* fall through to JSON_FLOAT */ > + } > case JSON_FLOAT: > /* FIXME dependent on locale */ You know, strtoll() also is dependent on locale (that is, non-C locale can accept strings that the C locale rejects), if you want to repeat this comment earlier for case JSON_INTEGER. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature