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

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> 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.

Misunderstanding.  I meant to say something like "defining an
enumeration with two values is usually silly, but you're going to add to
this one, so it's okay".

>> >  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.

I'm afraid prepending leads to inferior error messages.  What exactly do
you propose to prepend?

Can you point to a call of qnum_get_int() where the appropriate name
isn't readily available?

If callers of qnum_get_int() have to mess with the error message to get
a decent one, chances are qnum_get_int() should leave creating the Error
object to its callers by returning just an error code.  If many callers
need the same messages, they can share a wrapper.

>> >  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.

I dislike duplicating the (small) switch less than the clumsy signalling
of failure by giving the caller an Error object to free.  Matter of
taste.  However, it adds to my uneasy feeling about the qnum_get_int()
interface.

Further up, we discussed qnum_get_int() leaving error messages to
callers by returning just an error code.  Would also eliminate the
clumsiness here.  Hmm.

>> >
>> > -    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.

What exactly does g_assert() buy us over plain assert(), and
g_assert_not_reached() over assert(0)?

qapi/ overwhelmingly uses assert().