Re: [Qemu-devel] [PATCH 2/7] qapi: QMP input visitor, handle floats parsed as ints

[Luiz Capitulino]

On Fri, 11 May 2012 12:04:47 -0500
Michael Roth <address@hidden> wrote:

> On Fri, May 11, 2012 at 01:22:33PM -0300, Luiz Capitulino wrote:
> > On Fri, 27 Apr 2012 15:21:18 -0500
> > Michael Roth <address@hidden> wrote:
> > 
> > > JSON numbers can be interpreted as either integers or floating point
> > > values depending on their representation. As a result, QMP input visitor
> > > might visit a QInt when it was expecting a QFloat, so add handling to
> > > account for this.
> > > 
> > > Signed-off-by: Michael Roth <address@hidden>
> > > ---
> > >  qapi/qmp-input-visitor.c |    9 +++++++--
> > >  1 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> > > index 4cdc47d..bc91134 100644
> > > --- a/qapi/qmp-input-visitor.c
> > > +++ b/qapi/qmp-input-visitor.c
> > > @@ -246,13 +246,18 @@ static void qmp_input_type_number(Visitor *v, 
> > > double *obj, const char *name,
> > >      QmpInputVisitor *qiv = to_qiv(v);
> > >      QObject *qobj = qmp_input_get_object(qiv, name);
> > >  
> > > -    if (!qobj || qobject_type(qobj) != QTYPE_QFLOAT) {
> > > +    if (!qobj || (qobject_type(qobj) != QTYPE_QFLOAT &&
> > > +        qobject_type(qobj) != QTYPE_QINT)) {
> > >          error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : 
> > > "null",
> > >                    "double");
> > 
> > s/double/number
> > 
> > It's also important to note that migrate_set_downtime is (positively) 
> > affected
> > by this change. Today, it only accepts a double, eg.:
> > 
> > { "execute": "migrate_set_downtime", "arguments": { "value": 1 } }
> > {
> >     "error": {
> >         "class": "InvalidParameterType", 
> >         "desc": "Invalid parameter type for 'value', expected: double", 
> >         "data": {
> >             "name": "value", 
> >             "expected": "double"
> >         }
> >     }
> > }
> > 
> > That's a bug (as it's documented to accept a number) and this patch fixes 
> > it.
> > There's an interface change, but I think it won't cause problems in 
> > practice.
> > 
> > Please, fix the error message above and would be nice to get this (and patch
> > 02/07) as a separate series for 1.1.
> 
> Hmm, this is kinda awkward because I don't think we can change it without
> breaking any trees based off qom-next.
> 
> Since the error msg predates the bug fix (it just becomes more obviously
> wrong as a result of the fix), can you pull these commits as is into your
> QMP tree?
> 
> In the meantime I can send another patch, based on qom-next or qmp, that
> fixes the error msg. We can then get all 3 into 1.1/master via QMP tree, and I
> think qom-next should still merge cleanly back into master after 1.1

I think Andreas answered you here (in the sense that it's ok to respin this
patch), but I also would like to add that patch 4/7 doesn't seem a bug fix
to me (at least not worth it for 1.1).