| Message ID | 1479874588-1969-2-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
On 11/23/2016 07:45 AM, Markus Armbruster wrote: > 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(-) > Commit message claims to change "the few callers", patch changes just > one. Which of the two is right? Rebase churn - there were four callers of qobject_from_jsonf() that I originally identified, then I split it up into the one caller that used %PRId64 vs. the other three that can be done differently. > > 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? For 2.8, I can leave %lld support in qobject_from_jsonf(), and just cast to long long instead of int64_t. I'd still like to propose the series that kills all dynamic JSON for 2.9, but I don't mind posting a v2 of this series.
On 11/23/2016 07:56 AM, Eric Blake wrote: >> >> 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, Note that printf likewise lacks such conversions, so... >> we convert to int64_t >> first, then use PRId64. ...some conversion is necessary no matter what, even if it is to 'long long' instead of 'int64_t'. Also, our choice of using -1 as the sentinel for failures is difficult to portably code if we don't know if time_t is signed or unsigned (if it is a 32-bit unsigned value, widening it to long long won't give us our desired value), so it is not as simple as doing tv.tv_sec = -1; we still need a conditional or temporary.
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); } /*
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(-)