Re: [Qemu-devel] [PATCH 04/17] qapi: merge QInt and QFloat in QNum

[Marc-André Lureau]

Hi

On Thu, May 11, 2017 at 6:30 PM Markus Armbruster <address@hidden> wrote:

> Marc-André Lureau <address@hidden> writes:
>
>
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.1
> or later.
> > + * See the COPYING.LIB file in the top-level directory.
> > + */
> > +
> > +#ifndef QNUM_H
> > +#define QNUM_H
> > +
> > +#include "qapi/qmp/qobject.h"
> > +
> > +typedef enum {
> > +    QNUM_I64,
> > +    QNUM_DOUBLE
> > +} QNumType;
>
> Not bool because you're going to add to it.  Good.
>
>
Hmm? There is no plan to add bool there so far, I am not sure that makes
sense.


>  static void qobject_input_type_int64(Visitor *v, const char *name,
> int64_t *obj,
> > @@ -387,19 +384,19 @@ static void qobject_input_type_int64(Visitor *v,
> const char *name, int64_t *obj,
> >  {
> >      QObjectInputVisitor *qiv = to_qiv(v);
> >      QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
> > -    QInt *qint;
> > +    QNum *qnum;
> >
> >      if (!qobj) {
> >          return;
> >      }
> > -    qint = qobject_to_qint(qobj);
> > -    if (!qint) {
> > +    qnum = qobject_to_qnum(qobj);
> > +    if (!qnum) {
> >          error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
> >                     full_name(qiv, name), "integer");
> >          return;
> >      }
> >
> > -    *obj = qint_get_int(qint);
> > +    *obj = qnum_get_int(qnum, errp);
>
> Compare the error message a few lines up
>
>     Invalid parameter type for 'NAME', expected: integer
>
> to the one we get here:
>
>     The number is a float
>
> Which number?  There can be quite a few in a QMP command.  We need to
> pass @name to qnum_get_int() for a decent error message, like
>
>     Parameter 'NAME' is not a 64 bit integer
>
>
I suggest we do it the same way as qom/object.c, handle the error and
prepend/modify it. Passing @name feels awkward, since qtypes don't have to
have names.


> >  int64_t qdict_get_int(const QDict *qdict, const char *key)
> >  {
> > -    return qint_get_int(qobject_to_qint(qdict_get(qdict, key)));
> > +    return qnum_get_int(qobject_to_qnum(qdict_get(qdict, key)),
> &error_abort);
>
> Line is a bit long for my taste.
>
> >  }
> >
> >  /**
> > @@ -260,15 +248,25 @@ const char *qdict_get_str(const QDict *qdict,
> const char *key)
> >   * qdict_get_try_int(): Try to get integer mapped by 'key'
> >   *
> >   * Return integer mapped by 'key', if it is not present in
> > - * the dictionary or if the stored object is not of QInt type
> > + * the dictionary or if the stored object is not of QNum type
>
> Actually "is not a QNum representing an integer".
>
> >   * 'def_value' will be returned.
> >   */
> >  int64_t qdict_get_try_int(const QDict *qdict, const char *key,
> >                            int64_t def_value)
> >  {
> > -    QInt *qint = qobject_to_qint(qdict_get(qdict, key));
> > +    Error *err = NULL;
> > +    QNum *qnum = qobject_to_qnum(qdict_get(qdict, key));
> > +    int64_t val = def_value;
> > +
> > +    if (qnum) {
> > +        val = qnum_get_int(qnum, &err);
> > +    }
> > +    if (err) {
> > +        error_free(err);
> > +        val = def_value;
> > +    }
>
> This is a bit inefficient: it creates and destroys an Error object.
> Easily avoided with a predicate qnum_is_int().
>

True, but a predicate would have to deal with the same implicit conversion
as qnum_get_int(). Not sure it's worth the duplication.


>
> >
> > -    return qint ? qint_get_int(qint) : def_value;
> > +    return val;
> >  }
> >
> >  /**
> > diff --git a/qobject/qfloat.c b/qobject/qfloat.c
> > deleted file mode 100644
> > index d5da847701..0000000000
> > --- a/qobject/qfloat.c
> > +++ /dev/null
> > @@ -1,62 +0,0 @@
> > -/*
> > - * QFloat Module
> > - *
> > - * Copyright IBM, Corp. 2009
> > - *
> > - * Authors:
> > - *  Anthony Liguori   <address@hidden>
> > - *
> > - * This work is licensed under the terms of the GNU LGPL, version 2.1
> or later.
> > - * See the COPYING.LIB file in the top-level directory.
> > - *
> > - */
> > -
> > -#include "qemu/osdep.h"
> > -#include "qapi/qmp/qfloat.h"
> > -#include "qapi/qmp/qobject.h"
> > -#include "qemu-common.h"
> > -
> > -/**
> > - * qfloat_from_int(): Create a new QFloat from a float
> > - *
> > - * Return strong reference.
> > - */
> > -QFloat *qfloat_from_double(double value)
> > -{
> > -    QFloat *qf;
> > -
> > -    qf = g_malloc(sizeof(*qf));
> > -    qobject_init(QOBJECT(qf), QTYPE_QFLOAT);
> > -    qf->value = value;
> > -
> > -    return qf;
> > -}
> > -
> > -/**
> > - * qfloat_get_double(): Get the stored float
> > - */
> > -double qfloat_get_double(const QFloat *qf)
> > -{
> > -    return qf->value;
> > -}
> > -
> > -/**
> > - * qobject_to_qfloat(): Convert a QObject into a QFloat
> > - */
> > -QFloat *qobject_to_qfloat(const QObject *obj)
> > -{
> > -    if (!obj || qobject_type(obj) != QTYPE_QFLOAT) {
> > -        return NULL;
> > -    }
> > -    return container_of(obj, QFloat, base);
> > -}
> > -
> > -/**
> > - * qfloat_destroy_obj(): Free all memory allocated by a
> > - * QFloat object
> > - */
> > -void qfloat_destroy_obj(QObject *obj)
> > -{
> > -    assert(obj != NULL);
> > -    g_free(qobject_to_qfloat(obj));
> > -}
> > diff --git a/qobject/qint.c b/qobject/qint.c
> > deleted file mode 100644
> > index d7d1b3021f..0000000000
> > --- a/qobject/qint.c
> > +++ /dev/null
> > @@ -1,61 +0,0 @@
> > -/*
> > - * QInt Module
> > - *
> > - * Copyright (C) 2009 Red Hat Inc.
> > - *
> > - * Authors:
> > - *  Luiz Capitulino <address@hidden>
> > - *
> > - * This work is licensed under the terms of the GNU LGPL, version 2.1
> or later.
> > - * See the COPYING.LIB file in the top-level directory.
> > - */
> > -
> > -#include "qemu/osdep.h"
> > -#include "qapi/qmp/qint.h"
> > -#include "qapi/qmp/qobject.h"
> > -#include "qemu-common.h"
> > -
> > -/**
> > - * qint_from_int(): Create a new QInt from an int64_t
> > - *
> > - * Return strong reference.
> > - */
> > -QInt *qint_from_int(int64_t value)
> > -{
> > -    QInt *qi;
> > -
> > -    qi = g_malloc(sizeof(*qi));
> > -    qobject_init(QOBJECT(qi), QTYPE_QINT);
> > -    qi->value = value;
> > -
> > -    return qi;
> > -}
> > -
> > -/**
> > - * qint_get_int(): Get the stored integer
> > - */
> > -int64_t qint_get_int(const QInt *qi)
> > -{
> > -    return qi->value;
> > -}
> > -
> > -/**
> > - * qobject_to_qint(): Convert a QObject into a QInt
> > - */
> > -QInt *qobject_to_qint(const QObject *obj)
> > -{
> > -    if (!obj || qobject_type(obj) != QTYPE_QINT) {
> > -        return NULL;
> > -    }
> > -    return container_of(obj, QInt, base);
> > -}
> > -
> > -/**
> > - * qint_destroy_obj(): Free all memory allocated by a
> > - * QInt object
> > - */
> > -void qint_destroy_obj(QObject *obj)
> > -{
> > -    assert(obj != NULL);
> > -    g_free(qobject_to_qint(obj));
> > -}
> > diff --git a/qobject/qjson.c b/qobject/qjson.c
> > index b2f3bfec53..2e0930884e 100644
> > --- a/qobject/qjson.c
> > +++ b/qobject/qjson.c
> > @@ -132,12 +132,11 @@ static void to_json(const QObject *obj, QString
> *str, int pretty, int indent)
> >      case QTYPE_QNULL:
> >          qstring_append(str, "null");
> >          break;
> > -    case QTYPE_QINT: {
> > -        QInt *val = qobject_to_qint(obj);
> > -        char buffer[1024];
> > -
> > -        snprintf(buffer, sizeof(buffer), "%" PRId64, qint_get_int(val));
> > +    case QTYPE_QNUM: {
> > +        QNum *val = qobject_to_qnum(obj);
> > +        char *buffer = qnum_to_string(val);
> >          qstring_append(str, buffer);
> > +        g_free(buffer);
> >          break;
> >      }
> >      case QTYPE_QSTRING: {
> > @@ -234,34 +233,6 @@ static void to_json(const QObject *obj, QString
> *str, int pretty, int indent)
> >          qstring_append(str, "]");
> >          break;
> >      }
> > -    case QTYPE_QFLOAT: {
> > -        QFloat *val = qobject_to_qfloat(obj);
> > -        char buffer[1024];
> > -        int len;
> > -
> > -        /* FIXME: snprintf() is locale dependent; but JSON requires
> > -         * numbers to be formatted as if in the C locale. Dependence
> > -         * on C locale is a pervasive issue in QEMU. */
> > -        /* FIXME: This risks printing Inf or NaN, which are not valid
> > -         * JSON values. */
> > -        /* FIXME: the default precision of 6 for %f often causes
> > -         * rounding errors; we should be using DBL_DECIMAL_DIG (17),
> > -         * and only rounding to a shorter number if the result would
> > -         * still produce the same floating point value.  */
> > -        len = snprintf(buffer, sizeof(buffer), "%f",
> qfloat_get_double(val));
> > -        while (len > 0 && buffer[len - 1] == '0') {
> > -            len--;
> > -        }
> > -
> > -        if (len && buffer[len - 1] == '.') {
> > -            buffer[len - 1] = 0;
> > -        } else {
> > -            buffer[len] = 0;
> > -        }
> > -
> > -        qstring_append(str, buffer);
> > -        break;
> > -    }
> >      case QTYPE_QBOOL: {
> >          QBool *val = qobject_to_qbool(obj);
> >
> > diff --git a/qobject/qnum.c b/qobject/qnum.c
> > new file mode 100644
> > index 0000000000..8e9dd38350
> > --- /dev/null
> > +++ b/qobject/qnum.c
> > @@ -0,0 +1,138 @@
> > +/*
> > + * QNum Module
> > + *
> > + * Copyright (C) 2009 Red Hat Inc.
> > + *
> > + * Authors:
> > + *  Luiz Capitulino <address@hidden>
> > + *  Marc-André Lureau <address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.1
> or later.
> > + * See the COPYING.LIB file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "qapi/qmp/qnum.h"
> > +#include "qapi/qmp/qobject.h"
> > +#include "qemu-common.h"
> > +
> > +/**
> > + * qnum_from_int(): Create a new QNum from an int64_t
> > + *
> > + * Return strong reference.
> > + */
> > +QNum *qnum_from_int(int64_t value)
> > +{
> > +    QNum *qn = g_new(QNum, 1);
> > +
> > +    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > +    qn->type = QNUM_I64;
> > +    qn->u.i64 = value;
> > +
> > +    return qn;
> > +}
> > +
> > +/**
> > + * qnum_from_double(): Create a new QNum from a double
> > + *
> > + * Return strong reference.
> > + */
> > +QNum *qnum_from_double(double value)
> > +{
> > +    QNum *qn = g_new(QNum, 1);
> > +
> > +    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> > +    qn->type = QNUM_DOUBLE;
> > +    qn->u.dbl = value;
> > +
> > +    return qn;
> > +}
> > +
> > +/**
> > + * qnum_get_int(): Get an integer representation of the number
> > + */
> > +int64_t qnum_get_int(const QNum *qn, Error **errp)
> > +{
> > +    switch (qn->type) {
> > +    case QNUM_I64:
> > +        return qn->u.i64;
> > +    case QNUM_DOUBLE:
> > +        error_setg(errp, "The number is a float");
> > +        return 0;
> > +    }
> > +
> > +    g_assert_not_reached();
>
> g_assert_not_reached() is problematic, see "[PATCH] checkpatch: Disallow
> glib asserts in main code".
>
> Message-Id: <address@hidden>
> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg05499.html
>
>
Actually g_assert() and g_assert_not_reached() are accepted.
-- 
Marc-André Lureau