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