Hi
On Tue, May 16, 2017 at 9:33 PM Markus Armbruster <arm...@redhat.com> wrote:
> On the subject: there is no such thing as "QUInt". I guess you mean
> "uint type" (like in PATCH 06's subject). Could also say "QNUM_U64".
>
> Apropos subject: humor me, and start your subjects with a capital
> letter, like this:
>
> qapi: Update the qobject visitor ...
>
> Marc-André Lureau <marcandre.lur...@redhat.com> writes:
>
> > Switch to use QNum/uint where appropriate to remove i64 limitation.
> >
> > The input visitor will cast i64 input to u64 for compatibility
> > reasons (existing json QMP client already use negative i64 for large
> > u64, and expect an implicit cast in qemu).
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > qapi/qobject-input-visitor.c | 13 +++++++++++--
> > qapi/qobject-output-visitor.c | 3 +--
> > tests/test-qobject-output-visitor.c | 21 ++++++++++++++++-----
> > 3 files changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> > index 785949ebab..72cefcf677 100644
> > --- a/qapi/qobject-input-visitor.c
> > +++ b/qapi/qobject-input-visitor.c
> > @@ -420,9 +420,9 @@ static void qobject_input_type_int64_keyval(Visitor
> *v, const char *name,
> > static void qobject_input_type_uint64(Visitor *v, const char *name,
> > uint64_t *obj, Error **errp)
> > {
> > - /* FIXME: qobject_to_qnum mishandles values over INT64_MAX */
> > QObjectInputVisitor *qiv = to_qiv(v);
> > QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
> > + Error *err = NULL;
> > QNum *qnum;
> >
> > if (!qobj) {
> > @@ -435,7 +435,16 @@ static void qobject_input_type_uint64(Visitor *v,
> const char *name,
> > return;
> > }
> >
> > - *obj = qnum_get_int(qnum, errp);
> > + /* XXX: compatibility case, accept negative values as u64 */
>
> What does "XXX" signify?
>
It's a fairly common marker for something similar to FIXME (there are
hundreds of them in qemu source tree).
I'd like to leave a fixme that means that there should be a visitor
flag/capability to fix this compatibility behaviour. (this could be exposed
as a qmp capability)
>
> > + *obj = qnum_get_int(qnum, &err);
> > +
>
> Shouldn't the comment go right here?
>
>
Above qnum_get_int() is the right place imho.
> > + if (err) {
> > + error_free(err);
> > + err = NULL;
> > + *obj = qnum_get_uint(qnum, &err);
> > + }
> > +
> > + error_propagate(errp, err);
> > }
> >
> > static void qobject_input_type_uint64_keyval(Visitor *v, const char
> *name,
> > diff --git a/qapi/qobject-output-visitor.c
> b/qapi/qobject-output-visitor.c
> > index 2ca5093b22..70be84ccb5 100644
> > --- a/qapi/qobject-output-visitor.c
> > +++ b/qapi/qobject-output-visitor.c
> > @@ -150,9 +150,8 @@ static void qobject_output_type_int64(Visitor *v,
> const char *name,
> > static void qobject_output_type_uint64(Visitor *v, const char *name,
> > uint64_t *obj, Error **errp)
> > {
> > - /* FIXME values larger than INT64_MAX become negative */
> > QObjectOutputVisitor *qov = to_qov(v);
> > - qobject_output_add(qov, name, qnum_from_int(*obj));
> > + qobject_output_add(qov, name, qnum_from_uint(*obj));
>
> Before the patch, uint64_t values above INT64_MAX are sent as negative
> values, e.g. UINT64_MAX is sent as -1.
>
> After the patch, they are sent unmodified. Clearly a bug fix, but we
> have to consider compatibility issues anyway. Does libvirt expect large
> integers to be sent as negative integers? Does it cope with this fix
> gracefully? Eric, any idea?
>
The libvirt json parser seems to rely on virStrToLong_ui(), which is a
wrapper around strtoul(), so it accepts negative values with -2^63..-1.
Changing it to return values larger than INT64_MAX should be ok.
>
> > }
> >
> > static void qobject_output_type_bool(Visitor *v, const char *name, bool
> *obj,
> > diff --git a/tests/test-qobject-output-visitor.c
> b/tests/test-qobject-output-visitor.c
> > index 66a682d5a8..767818e393 100644
> > --- a/tests/test-qobject-output-visitor.c
> > +++ b/tests/test-qobject-output-visitor.c
> > @@ -595,15 +595,26 @@ static void check_native_list(QObject *qobj,
> > qlist = qlist_copy(qobject_to_qlist(qdict_get(qdict, "data")));
> >
> > switch (kind) {
> > - case USER_DEF_NATIVE_LIST_UNION_KIND_S8:
> > - case USER_DEF_NATIVE_LIST_UNION_KIND_S16:
> > - case USER_DEF_NATIVE_LIST_UNION_KIND_S32:
> > - case USER_DEF_NATIVE_LIST_UNION_KIND_S64:
> > case USER_DEF_NATIVE_LIST_UNION_KIND_U8:
> > case USER_DEF_NATIVE_LIST_UNION_KIND_U16:
> > case USER_DEF_NATIVE_LIST_UNION_KIND_U32:
> > case USER_DEF_NATIVE_LIST_UNION_KIND_U64:
> > - /* all integer elements in JSON arrays get stored into QNums
> when
> > + for (i = 0; i < 32; i++) {
> > + QObject *tmp;
> > + QNum *qvalue;
> > + tmp = qlist_peek(qlist);
> > + g_assert(tmp);
> > + qvalue = qobject_to_qnum(tmp);
> > + g_assert_cmpuint(qnum_get_uint(qvalue, &error_abort), ==,
> i);
> > + qobject_decref(qlist_pop(qlist));
> > + }
> > + break;
> > +
> > + case USER_DEF_NATIVE_LIST_UNION_KIND_S8:
> > + case USER_DEF_NATIVE_LIST_UNION_KIND_S16:
> > + case USER_DEF_NATIVE_LIST_UNION_KIND_S32:
> > + case USER_DEF_NATIVE_LIST_UNION_KIND_S64:
> > + /* all integer elements in JSON arrays get stored into QInts
> when
> > * we convert to QObjects, so we can check them all in the same
> > * fashion, so simply fall through here
> > */
>
> Make that "All signed integer ...", and wing both ends of the comment.
>
ok
> Or simply drop the comment.
--
Marc-André Lureau
