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

[Daniel P. Berrange]

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.
> > 
> > This makes it possible to use QmpInputVisitor with a
> > QDict produced from QemuOpts, where everything is in
> > string format.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  include/qapi/qmp-input-visitor.h |   3 +
> >  qapi/qmp-input-visitor.c         |  96 +++++++++++++++++++++++++++-----
> >  tests/test-qmp-input-visitor.c   | 115 
> > ++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 196 insertions(+), 18 deletions(-)


> > -    *obj = qint_get_int(qint);
> > +    qstr = qobject_to_qstring(qobj);
> > +    if (qstr && qstr->string && qiv->autocast) {
> > +        errno = 0;
> > +        if (qemu_strtoull(qstr->string, NULL, 10, obj) == 0) {
> 
> And again.
> 
> 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 ?

> [Ultimately, I'd like QInt to be enhanced to track whether the input was
> signed or unsigned, and automatically make the output match the input
> when converting back to string; that is, track 65 bits of information
> instead of 64; but that's no sooner than 2.7 material]
> 
> 
> >  static void qmp_input_type_bool(Visitor *v, const char *name, bool *obj,
> >                                  Error **errp)
> >  {
> >      QmpInputVisitor *qiv = to_qiv(v);
> > -    QBool *qbool = qobject_to_qbool(qmp_input_get_object(qiv, name, true));
> > +    QObject *qobj = qmp_input_get_object(qiv, name, true);
> > +    QBool *qbool;
> > +    QString *qstr;
> >  
> > -    if (!qbool) {
> > -        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> > -                   "boolean");
> > +    qbool = qobject_to_qbool(qobj);
> > +    if (qbool) {
> > +        *obj = qbool_get_bool(qbool);
> >          return;
> >      }
> >  
> > -    *obj = qbool_get_bool(qbool);
> > +
> > +    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

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|