| Message ID | 1335558083-26196-3-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 27 Apr 2012 15:21:18 -0500 Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com> > --- > 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. > return; > } > > - *obj = qfloat_get_double(qobject_to_qfloat(qobj)); > + if (qobject_type(qobj) == QTYPE_QINT) { > + *obj = qint_get_int(qobject_to_qint(qobj)); > + } else { > + *obj = qfloat_get_double(qobject_to_qfloat(qobj)); > + } > } > > static void qmp_input_start_optional(Visitor *v, bool *present,
On Fri, May 11, 2012 at 01:22:33PM -0300, Luiz Capitulino wrote: > On Fri, 27 Apr 2012 15:21:18 -0500 > Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com> > > --- > > 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 > > > > return; > > } > > > > - *obj = qfloat_get_double(qobject_to_qfloat(qobj)); > > + if (qobject_type(qobj) == QTYPE_QINT) { > > + *obj = qint_get_int(qobject_to_qint(qobj)); > > + } else { > > + *obj = qfloat_get_double(qobject_to_qfloat(qobj)); > > + } > > } > > > > static void qmp_input_start_optional(Visitor *v, bool *present, >
Am 11.05.2012 19:04, schrieb Michael Roth: > On Fri, May 11, 2012 at 01:22:33PM -0300, Luiz Capitulino wrote: >> On Fri, 27 Apr 2012 15:21:18 -0500 >> Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com> >>> --- >>> 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. That's no problem at all, it is announced as rebasing and I have scripts in place to pull and recursively rebase all my QOM branches. For such a trivial textual change I can even fix it up manually. :) Since Luiz is now taking care of this one I will simply drop it from my qom-1.1 branch to avoid any confusion. Andreas > 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 > >> >> >>> return; >>> } >>> >>> - *obj = qfloat_get_double(qobject_to_qfloat(qobj)); >>> + if (qobject_type(qobj) == QTYPE_QINT) { >>> + *obj = qint_get_int(qobject_to_qint(qobj)); >>> + } else { >>> + *obj = qfloat_get_double(qobject_to_qfloat(qobj)); >>> + } >>> } >>> >>> static void qmp_input_start_optional(Visitor *v, bool *present,
On Fri, May 11, 2012 at 07:16:18PM +0200, Andreas Färber wrote: > Am 11.05.2012 19:04, schrieb Michael Roth: > > On Fri, May 11, 2012 at 01:22:33PM -0300, Luiz Capitulino wrote: > >> On Fri, 27 Apr 2012 15:21:18 -0500 > >> Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com> > >>> --- > >>> 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. > > That's no problem at all, it is announced as rebasing and I have scripts > in place to pull and recursively rebase all my QOM branches. > > For such a trivial textual change I can even fix it up manually. :) > > Since Luiz is now taking care of this one I will simply drop it from my > qom-1.1 branch to avoid any confusion. That'll work, thanks. Luiz, I'll send a separate patch shortly with the error msg fixed. > > Andreas > > > 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 > > > >> > >> > >>> return; > >>> } > >>> > >>> - *obj = qfloat_get_double(qobject_to_qfloat(qobj)); > >>> + if (qobject_type(qobj) == QTYPE_QINT) { > >>> + *obj = qint_get_int(qobject_to_qint(qobj)); > >>> + } else { > >>> + *obj = qfloat_get_double(qobject_to_qfloat(qobj)); > >>> + } > >>> } > >>> > >>> static void qmp_input_start_optional(Visitor *v, bool *present, > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
On Fri, 11 May 2012 12:04:47 -0500 Michael Roth <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com> 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 <mdroth@linux.vnet.ibm.com> > > > --- > > > 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).
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"); return; } - *obj = qfloat_get_double(qobject_to_qfloat(qobj)); + if (qobject_type(qobj) == QTYPE_QINT) { + *obj = qint_get_int(qobject_to_qint(qobj)); + } else { + *obj = qfloat_get_double(qobject_to_qfloat(qobj)); + } } static void qmp_input_start_optional(Visitor *v, bool *present,
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 <mdroth@linux.vnet.ibm.com> --- qapi/qmp-input-visitor.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)