Re: [Qemu-devel] [PATCH 1/3] qmp-event: Avoid qobject_from_jsonf("%"PRId64)

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> The qobject_from_jsonf() function implements a pseudo-printf
> language for creating a QObject; however, it is hard-coded to
> only parse a subset of formats understood by printf().  In
> particular, any use of a 64-bit integer works only if the
> system's definition of PRId64 matches what the parser expects;
> which works on glibc (%lld) and mingw (%I64d), but not on
> Mac OS (%qd).  Rather than enhance the parser, it is just as
> easy to open-code the few callers that were relying on this
> particular conversion.
>
> Reported by: G 3 <programmingkidx@gmail.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/qmp-event.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/qapi/qmp-event.c b/qapi/qmp-event.c
> index 8bba165..26e10a1 100644
> --- a/qapi/qmp-event.c
> +++ b/qapi/qmp-event.c
> @@ -16,6 +16,8 @@
>  #include "qemu-common.h"
>  #include "qapi/qmp-event.h"
>  #include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qint.h"
>  #include "qapi/qmp/qjson.h"
>
>  static QMPEventFuncEmit qmp_emit;
> @@ -33,7 +35,7 @@ QMPEventFuncEmit qmp_event_get_func_emit(void)
>  static void timestamp_put(QDict *qdict)
>  {
>      int err;
> -    QObject *obj;
> +    QDict *stamp;
>      qemu_timeval tv;
>      int64_t sec, usec;
>
> @@ -47,10 +49,10 @@ static void timestamp_put(QDict *qdict)
>          usec = tv.tv_usec;
>      }
>
> -    obj = qobject_from_jsonf("{ 'seconds': %" PRId64 ", "
> -                             "'microseconds': %" PRId64 " }",
> -                             sec, usec);
> -    qdict_put_obj(qdict, "timestamp", obj);
> +    stamp = qdict_new();
> +    qdict_put(stamp, "seconds", qint_from_int(sec));
> +    qdict_put(stamp, "microseconds", qint_from_int(usec));
> +    qdict_put(qdict, "timestamp", stamp);
>  }
>
>  /*

Commit message claims to change "the few callers", patch changes just
one.  Which of the two is right?

In my opinion, the code becomes less readable.

We want to convert struct timeval members tv_sec (of type time_t) and
tv_usec (of type suseconds_t) here.  Since qobject_from_jsonf() lacks
conversion specifiers for time_t and suseconds_t, we convert to int64_t
first, then use PRId64.  The problem is that we don't actually implement
PRId64 everywhere.  Why not simply long long and %lld?