Re: [Qemu-devel] [PATCH 08/17] qapi: update the qobject visitor to use QUInt

[Marc-André Lureau]

Hi

On Tue, May 16, 2017 at 9:33 PM Markus Armbruster <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > ---
> >  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