[Qemu-devel] [PATCH for-2.8 0/3] Fix MacOS runtime failure of qobject_from_jsonf()

[Qemu-devel] [PATCH for-2.8 0/3] Fix MacOS runtime failure of qobject_from_jsonf()

[Eric Blake]

programmingkidx@gmail.com[*] reported a runtime failure on a
32-bit Mac OS compilation, where "%"PRId64 expands to "%qd".
Fortunately, we had very few spots that were relying on our
pseudo-printf JSON parsing of 64-bit numbers, so it was
easier to just convert callers and rip out the 64-bit support.
The remaining uses of pseudo-printf handling are more complex;
there are only 3 users in the released codebased, but LOTS of
users in the testsuite (via wrapper functions like qmp()); I
will be posting a followup series that rips out the remaining
uses of dynamic JSON, but it will be 2.9 material, while
these three patches qualify for 2.8.

[*] git log shows the name John, but the particular email that
sparked this only stated the non-descript name 'G 3', which
makes it a bit hard for me to know which form is preferred
when lending credit.

In patch 3, I chose to remove existing support for 64-bit
dynamic JSON on all platforms, so that we quickly see a failure
in an attempt to use such a construct even on mainstream glibc
development environments, rather than only on the less-frequent
32-bit Mac OS build environment.

Eric Blake (3):
  qmp-event: Avoid qobject_from_jsonf("%"PRId64)
  test-qga: Avoid qobject_from_jsonf("%"PRId64)
  qapi: Drop support for qobject_from_jsonf("%"PRId64)

 qapi/qmp-event.c                   | 12 +++++++-----
 qobject/json-lexer.c               | 28 ----------------------------
 qobject/json-parser.c              |  5 -----
 tests/check-qjson.c                | 10 ----------
 tests/test-qga.c                   |  9 +++++++--
 tests/test-qobject-input-visitor.c |  5 +++--
 6 files changed, 17 insertions(+), 52 deletions(-)

-- 
2.7.4

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

[Eric Blake]

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);
 }

 /*
-- 
2.7.4

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

[Markus Armbruster]

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?

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1915 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)

[Eric Blake]

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 use normal printf() for this particular conversion,
matching what is done elsewhere in this file.

Reported by: G 3 <programmingkidx@gmail.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/test-qga.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/test-qga.c b/tests/test-qga.c
index 40af649..421e27c 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -852,8 +852,13 @@ static void test_qga_guest_exec(gconstpointer fix)
     /* wait for completion */
     now = g_get_monotonic_time();
     do {
-        ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
-                     " 'arguments': { 'pid': %" PRId64 "  } }", pid);
+        char *cmd;
+
+        cmd = g_strdup_printf("{'execute': 'guest-exec-status',"
+                              " 'arguments': { 'pid': %" PRId64 " } }",
+                              pid);
+        ret = qmp_fd(fixture->fd, cmd);
+        g_free(cmd);
         g_assert_nonnull(ret);
         val = qdict_get_qdict(ret, "return");
         exited = qdict_get_bool(val, "exited");
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)

[Paolo Bonzini]

----- Original Message -----
> From: "Eric Blake" <eblake@redhat.com>
> To: qemu-devel@nongnu.org
> Cc: programmingkidx@gmail.com, armbru@redhat.com, pbonzini@redhat.com
> Sent: Wednesday, November 23, 2016 5:16:27 AM
> Subject: [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)
> 
> 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 use normal printf() for this particular conversion,
> matching what is done elsewhere in this file.
> 
> Reported by: G 3 <programmingkidx@gmail.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-qga.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index 40af649..421e27c 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -852,8 +852,13 @@ static void test_qga_guest_exec(gconstpointer fix)
>      /* wait for completion */
>      now = g_get_monotonic_time();
>      do {
> -        ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
> -                     " 'arguments': { 'pid': %" PRId64 "  } }", pid);
> +        char *cmd;
> +
> +        cmd = g_strdup_printf("{'execute': 'guest-exec-status',"
> +                              " 'arguments': { 'pid': %" PRId64 " } }",
> +                              pid);

This is too ugly to see. :)  Why not use %lld, or just make pid an
int?  Are there really systems with 64-bit pid_t?

Paolo

> +        ret = qmp_fd(fixture->fd, cmd);
> +        g_free(cmd);
>          g_assert_nonnull(ret);
>          val = qdict_get_qdict(ret, "return");
>          exited = qdict_get_bool(val, "exited");
> --
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3665 bytes --]

On 11/23/2016 03:23 AM, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
>> From: "Eric Blake" <eblake@redhat.com>
>> To: qemu-devel@nongnu.org
>> Cc: programmingkidx@gmail.com, armbru@redhat.com, pbonzini@redhat.com
>> Sent: Wednesday, November 23, 2016 5:16:27 AM
>> Subject: [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)
>>
>> 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 use normal printf() for this particular conversion,
>> matching what is done elsewhere in this file.

This is the key, look at:
$ git grep -A2 strdup_printf tests/test-qga.c

and you'll see that my change is no grosser than the rest of the file.

>> -        ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
>> -                     " 'arguments': { 'pid': %" PRId64 "  } }", pid);
>> +        char *cmd;
>> +
>> +        cmd = g_strdup_printf("{'execute': 'guest-exec-status',"
>> +                              " 'arguments': { 'pid': %" PRId64 " } }",
>> +                              pid);

_THIS_ patch merely moves the (pre-existing, ugly) association of
"%"PRId64, pid from qobject_from_jsonf() to g_strdup_printf().  But your
question...

> 
> This is too ugly to see. :)  Why not use %lld, or just make pid an
> int?  Are there really systems with 64-bit pid_t?

...is whether the pre-existing code is correct.  For the purposes of
fixing Mac OS compilation, I argue that your question doesn't matter.  I
agree with you that the code looks gross, but that's not my fault.  But
here's why I think changing the ugliness is wrong:

The existing code already used 64-bit pid (note, this is 'pid', not
'pid_t'), because of:

    int64_t pid, now, exitcode;
...
    val = qdict_get_qdict(ret, "return");
    pid = qdict_get_int(val, "pid");
    g_assert_cmpint(pid, >, 0);

And yes, the qapi schema currently lists a 64-bit type (anything without
an explicit size is 64 bits over JSON):

{ 'struct': 'GuestExec',
  'data': { 'pid': 'int'} }

And yes, 64-bit mingw has a 64-bit pid_t (ewwww)

/usr/x86_64-w64-mingw32/sys-root/mingw/include/sys/types.h:
#ifndef _PID_T_
#define _PID_T_
#ifndef _WIN64
typedef int     _pid_t;
#else
__MINGW_EXTENSION
typedef __int64 _pid_t;
#endif

although there is a long-standing bug in mingw headers, since getpid()
returns a different type than pid_t:

/usr/x86_64-w64-mingw32/sys-root/mingw/include/unistd.h:
  int __cdecl getpid(void) __MINGW_ATTRIB_DEPRECATED_MSVC2005;

and sadly, mingw lacks the POSIX-required id_t type (which must be at
least as large as any of pid_t, uid_t, or gid_t) to know if it was
intentional.  At any rate, I worry that changing the QGA definition to
specify 'int32' may break compilation on mingw, where the qga code would
have to start casting from the (possibly-broken) 64-bit pid_t down to a
32-bit value when populating the QAPI struct to pass back over the wire.

I _really_ wish mingw would fix their headers (decide if 64-bit pid_t is
really correct or not, and then make pid_t and getpid() match as well as
supply id_t), but this isn't the forum to get that accomplished.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

On 11/23/2016 04:36 AM, Eric Blake wrote:

> I _really_ wish mingw would fix their headers (decide if 64-bit pid_t is
> really correct or not, and then make pid_t and getpid() match as well as
> supply id_t), but this isn't the forum to get that accomplished.

https://bugzilla.redhat.com/show_bug.cgi?id=1397787

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 285 bytes --]



On 23/11/2016 11:36, Eric Blake wrote:
> This is the key, look at:
> $ git grep -A2 strdup_printf tests/test-qga.c
> 
> and you'll see that my change is no grosser than the rest of the file.
> 

Oh well... I guess with a better commit message the patch is fine.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)

[Markus Armbruster]

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 use normal printf() for this particular conversion,
> matching what is done elsewhere in this file.
>
> Reported by: G 3 <programmingkidx@gmail.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-qga.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index 40af649..421e27c 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -852,8 +852,13 @@ static void test_qga_guest_exec(gconstpointer fix)
>      /* wait for completion */
>      now = g_get_monotonic_time();
>      do {
> -        ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
> -                     " 'arguments': { 'pid': %" PRId64 "  } }", pid);
> +        char *cmd;
> +
> +        cmd = g_strdup_printf("{'execute': 'guest-exec-status',"
> +                              " 'arguments': { 'pid': %" PRId64 " } }",
> +                              pid);
> +        ret = qmp_fd(fixture->fd, cmd);
> +        g_free(cmd);
>          g_assert_nonnull(ret);
>          val = qdict_get_qdict(ret, "return");
>          exited = qdict_get_bool(val, "exited");

Same problem as in the previous patch, but here you replace it by
g_strdup_printf(), where the previous patch replaced it by manual
QObject construction,

Manual QObject construction tends to be less readable.

g_strdup_printf() doesn't have that problem, but it has a more serious
one: escaping for JSON is no longer below the hood.

Since the string gets passed to qmp_fd(), we additionally need to escape
'%'.

Interfaces that require callers to escape almost inevitably result in
bugs if experience is any guide.  Safer, less low level interfaces are
preferable.

Nothing actually needs escaping here, so your code isn't wrong.  It's
just a bad example.

You've pointed out that the file is chock-full of bad examples already,
so one more won't make a difference.  Point taken regarding the
immediate fix.  But I doubt it a sane strategy for replacing _jsonf().

Re: [Qemu-devel] [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On 11/23/2016 08:05 AM, Markus Armbruster wrote:

> Same problem as in the previous patch, but here you replace it by
> g_strdup_printf(), where the previous patch replaced it by manual
> QObject construction,
> 
> Manual QObject construction tends to be less readable.

Are there things we can do to make it more readable to the point where
it would be tolerable in the situations where it is needed?

One of the patches on my dynamic-JSON removal series adds a new:

qdict_put_int(dict, "key", 1);

which is a lot more legible than:

qdict_put(dict, "key", qint_from_int(1));


> 
> g_strdup_printf() doesn't have that problem, but it has a more serious
> one: escaping for JSON is no longer below the hood.
> 
> Since the string gets passed to qmp_fd(), we additionally need to escape
> '%'.

Worse, the escaping of %s differs between the two (in printf, %s just
concatenates strings, in dynamic JSON, it adds outer "" and escapes
inner " into \").

> 
> Interfaces that require callers to escape almost inevitably result in
> bugs if experience is any guide.  Safer, less low level interfaces are
> preferable.
> 
> Nothing actually needs escaping here, so your code isn't wrong.  It's
> just a bad example.
> 
> You've pointed out that the file is chock-full of bad examples already,
> so one more won't make a difference.  Point taken regarding the
> immediate fix.  But I doubt it a sane strategy for replacing _jsonf().
> 

Well, until I post my conversion series that eliminates _json[fv](), we
don't have any hard numbers on how many bad examples remain, or whether
the cleanup looks worth it.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/23/2016 08:05 AM, Markus Armbruster wrote:
>
>> Same problem as in the previous patch, but here you replace it by
>> g_strdup_printf(), where the previous patch replaced it by manual
>> QObject construction,
>> 
>> Manual QObject construction tends to be less readable.
>
> Are there things we can do to make it more readable to the point where
> it would be tolerable in the situations where it is needed?
>
> One of the patches on my dynamic-JSON removal series adds a new:
>
> qdict_put_int(dict, "key", 1);
>
> which is a lot more legible than:
>
> qdict_put(dict, "key", qint_from_int(1));

It's more legible, but I wouldn't call it "a lot more legible".

>> g_strdup_printf() doesn't have that problem, but it has a more serious
>> one: escaping for JSON is no longer below the hood.
>> 
>> Since the string gets passed to qmp_fd(), we additionally need to escape
>> '%'.
>
> Worse, the escaping of %s differs between the two (in printf, %s just
> concatenates strings, in dynamic JSON, it adds outer "" and escapes
> inner " into \").

That's a feature.  It actually escapes much more than just '"'.  Have a
look at to_json() case QTYPE_QSTRING.

The imporant bit here is: _jsonf() is not printf()!  The part it shares
with printf() is the argument types associated with conversion
specifiers, and it shares them just because that way the compiler can
help us catch type errors.  What it does with the arguments is
*different*, because what it does is different.  It does *not* format a
string.  Not even conceptually.  It builds a QObject from a string
template.

>> Interfaces that require callers to escape almost inevitably result in
>> bugs if experience is any guide.  Safer, less low level interfaces are
>> preferable.
>> 
>> Nothing actually needs escaping here, so your code isn't wrong.  It's
>> just a bad example.
>> 
>> You've pointed out that the file is chock-full of bad examples already,
>> so one more won't make a difference.  Point taken regarding the
>> immediate fix.  But I doubt it a sane strategy for replacing _jsonf().
>
> Well, until I post my conversion series that eliminates _json[fv](), we
> don't have any hard numbers on how many bad examples remain, or whether
> the cleanup looks worth it.

Yes.  Without patches, the discussion is speculative.

[Qemu-devel] [PATCH 3/3] qapi: Drop support for qobject_from_jsonf("%"PRId64)

[Eric Blake]

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, we have already
converted almost all clients to use an alternative method;
convert or eliminate the remaining uses in the testsuite, and
rip out this code from the parser.

Ripping it all out means that we will now uniformly get
failures on all platforms that try to use dynamic JSON with
64-bit numbers. Ultimately, I plan for later patches to rip
out dynamic JSON altogether, but that is more invasive and
therefore not appropriate for the 2.8 release, while this
patch fixes an actual testsuite failure of check-qjson on
Mac OS.

Reported by: G 3 <programmingkidx@gmail.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qobject/json-lexer.c               | 28 ----------------------------
 qobject/json-parser.c              |  5 -----
 tests/check-qjson.c                | 10 ----------
 tests/test-qobject-input-visitor.c |  5 +++--
 4 files changed, 3 insertions(+), 45 deletions(-)

diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index af4a75e..b175da7 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -59,11 +59,6 @@ enum json_lexer_state {
     IN_NEG_NONZERO_NUMBER,
     IN_KEYWORD,
     IN_ESCAPE,
-    IN_ESCAPE_L,
-    IN_ESCAPE_LL,
-    IN_ESCAPE_I,
-    IN_ESCAPE_I6,
-    IN_ESCAPE_I64,
     IN_WHITESPACE,
     IN_START,
 };
@@ -225,35 +220,12 @@ static const uint8_t json_lexer[][256] =  {
     },

     /* escape */
-    [IN_ESCAPE_LL] = {
-        ['d'] = JSON_ESCAPE,
-    },
-
-    [IN_ESCAPE_L] = {
-        ['d'] = JSON_ESCAPE,
-        ['l'] = IN_ESCAPE_LL,
-    },
-
-    [IN_ESCAPE_I64] = {
-        ['d'] = JSON_ESCAPE,
-    },
-
-    [IN_ESCAPE_I6] = {
-        ['4'] = IN_ESCAPE_I64,
-    },
-
-    [IN_ESCAPE_I] = {
-        ['6'] = IN_ESCAPE_I6,
-    },
-
     [IN_ESCAPE] = {
         ['d'] = JSON_ESCAPE,
         ['i'] = JSON_ESCAPE,
         ['p'] = JSON_ESCAPE,
         ['s'] = JSON_ESCAPE,
         ['f'] = JSON_ESCAPE,
-        ['l'] = IN_ESCAPE_L,
-        ['I'] = IN_ESCAPE_I,
     },

     /* top level rule */
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index c18e48a..492d141 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -467,11 +467,6 @@ static QObject *parse_escape(JSONParserContext *ctxt, va_list *ap)
         return QOBJECT(qbool_from_bool(va_arg(*ap, int)));
     } else if (!strcmp(token->str, "%d")) {
         return QOBJECT(qint_from_int(va_arg(*ap, int)));
-    } else if (!strcmp(token->str, "%ld")) {
-        return QOBJECT(qint_from_int(va_arg(*ap, long)));
-    } else if (!strcmp(token->str, "%lld") ||
-               !strcmp(token->str, "%I64d")) {
-        return QOBJECT(qint_from_int(va_arg(*ap, long long)));
     } else if (!strcmp(token->str, "%s")) {
         return QOBJECT(qstring_from_str(va_arg(*ap, const char *)));
     } else if (!strcmp(token->str, "%f")) {
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 8595574..7149a92 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -964,7 +964,6 @@ static void vararg_number(void)
     QInt *qint;
     QFloat *qfloat;
     int value = 0x2342;
-    int64_t value64 = 0x2342342343LL;
     double valuef = 2.323423423;

     obj = qobject_from_jsonf("%d", value);
@@ -976,15 +975,6 @@ static void vararg_number(void)

     QDECREF(qint);

-    obj = qobject_from_jsonf("%" PRId64, value64);
-    g_assert(obj != NULL);
-    g_assert(qobject_type(obj) == QTYPE_QINT);
-
-    qint = qobject_to_qint(obj);
-    g_assert(qint_get_int(qint) == value64);
-
-    QDECREF(qint);
-
     obj = qobject_from_jsonf("%f", valuef);
     g_assert(obj != NULL);
     g_assert(qobject_type(obj) == QTYPE_QFLOAT);
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 26c5012..945404a 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -83,10 +83,11 @@ static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
 static void test_visitor_in_int(TestInputVisitorData *data,
                                 const void *unused)
 {
-    int64_t res = 0, value = -42;
+    int64_t res = 0;
+    int value = -42;
     Visitor *v;

-    v = visitor_input_test_init(data, "%" PRId64, value);
+    v = visitor_input_test_init(data, "%d", value);

     visit_type_int(v, NULL, &res, &error_abort);
     g_assert_cmpint(res, ==, value);
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] qapi: Drop support for qobject_from_jsonf("%"PRId64)

[Paolo Bonzini]

> 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, we have already
> converted almost all clients to use an alternative method;
> convert or eliminate the remaining uses in the testsuite, and
> rip out this code from the parser.
> 
> Ripping it all out means that we will now uniformly get
> failures on all platforms that try to use dynamic JSON with
> 64-bit numbers. Ultimately, I plan for later patches to rip
> out dynamic JSON altogether, but that is more invasive and
> therefore not appropriate for the 2.8 release, while this
> patch fixes an actual testsuite failure of check-qjson on
> Mac OS.
> 
> Reported by: G 3 <programmingkidx@gmail.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

This is throwing out the baby with the bathwater.  %lld works
just fine for long long.  Throwing away %I64d is fine though...

> @@ -964,7 +964,6 @@ static void vararg_number(void)
>      QInt *qint;
>      QFloat *qfloat;
>      int value = 0x2342;
> -    int64_t value64 = 0x2342342343LL;
>      double valuef = 2.323423423;
> 
>      obj = qobject_from_jsonf("%d", value);
> @@ -976,15 +975,6 @@ static void vararg_number(void)
> 
>      QDECREF(qint);
> 
> -    obj = qobject_from_jsonf("%" PRId64, value64);
> -    g_assert(obj != NULL);
> -    g_assert(qobject_type(obj) == QTYPE_QINT);
> -
> -    qint = qobject_to_qint(obj);
> -    g_assert(qint_get_int(qint) == value64);
> -
> -    QDECREF(qint);
> -
>      obj = qobject_from_jsonf("%f", valuef);
>      g_assert(obj != NULL);
>      g_assert(qobject_type(obj) == QTYPE_QFLOAT);

if you change the test to use %lld and long long instead of int64_t.

> diff --git a/tests/test-qobject-input-visitor.c
> b/tests/test-qobject-input-visitor.c
> index 26c5012..945404a 100644
> --- a/tests/test-qobject-input-visitor.c
> +++ b/tests/test-qobject-input-visitor.c
> @@ -83,10 +83,11 @@ static Visitor
> *visitor_input_test_init_raw(TestInputVisitorData *data,
>  static void test_visitor_in_int(TestInputVisitorData *data,
>                                  const void *unused)
>  {
> -    int64_t res = 0, value = -42;
> +    int64_t res = 0;
> +    int value = -42;
>      Visitor *v;
> 
> -    v = visitor_input_test_init(data, "%" PRId64, value);
> +    v = visitor_input_test_init(data, "%d", value);
> 
>      visit_type_int(v, NULL, &res, &error_abort);
>      g_assert_cmpint(res, ==, value);
> --
> 2.7.4
> 
> 

This part is fine I guess.

Paolo

Re: [Qemu-devel] [PATCH 3/3] qapi: Drop support for qobject_from_jsonf("%"PRId64)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5051 bytes --]

On 11/23/2016 03:25 AM, Paolo Bonzini wrote:
>> 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, we have already
>> converted almost all clients to use an alternative method;
>> convert or eliminate the remaining uses in the testsuite, and
>> rip out this code from the parser.
>>
>> Ripping it all out means that we will now uniformly get
>> failures on all platforms that try to use dynamic JSON with
>> 64-bit numbers. Ultimately, I plan for later patches to rip
>> out dynamic JSON altogether, but that is more invasive and
>> therefore not appropriate for the 2.8 release, while this
>> patch fixes an actual testsuite failure of check-qjson on
>> Mac OS.
>>
>> Reported by: G 3 <programmingkidx@gmail.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> This is throwing out the baby with the bathwater.  %lld works
> just fine for long long.  Throwing away %I64d is fine though...

On 64-bit systems where int64_t is 'long' rather than 'long long',
PRId64 is %ld, not %lld.  And on 32-bit MacOS, PRId64 is %qd, which is
NOT covered by the existing JSON parser.  If you write:

int64_t foo;
qobject_from_jsonf("%lld", foo)

then gcc will complain on all platforms where %lld is not the right
string to match against int64_t, thanks to -Wformat.  We could instead
write:

int64_t foo;
qobject_from_jsonf("%lld", (long long) foo);

and have that work everywhere, but then you have to explicitly cast.
And our current qdict_get_int() code returns int64_t, not long long, so
it's hard to convince people to write:

long long foo;
foo = qdict_get_int(...);
qobject_from_jsonf("%lld", foo);

since 'long long' is more typing than 'int64_t'. I suppose we could do
that, but my next question is why bother.  As I've proven in these three
patches, there were VERY FEW clients that were trying to use dynamic
JSON on a 64-bit value in the first place.  Ripping out ALL 64-bit
support _proves_ that we can't mess it up in the future, vs. leaving
%lld there and getting it right for some versions of glibc but still
failing on other platforms when someone uses PRId64 instead of an
explicit %lld.

My other argument is that I _do_ intend to rip out ALL of the dynamic
JSON support, at which point we no longer have %d, let along %lld.
Until you see that followup series and decide whether it was too
invasive for 2.9, it's hard to say that we are throwing out anything
useful in this short-term fix for 2.8.  So I guess that gives me a
reason to hurry up and finish my work on that series to post it today
before I take a long holiday weekend.

>> +++ b/tests/test-qobject-input-visitor.c
>> @@ -83,10 +83,11 @@ static Visitor
>> *visitor_input_test_init_raw(TestInputVisitorData *data,
>>  static void test_visitor_in_int(TestInputVisitorData *data,
>>                                  const void *unused)
>>  {
>> -    int64_t res = 0, value = -42;
>> +    int64_t res = 0;
>> +    int value = -42;
>>      Visitor *v;
>>
>> -    v = visitor_input_test_init(data, "%" PRId64, value);
>> +    v = visitor_input_test_init(data, "%d", value);
>>
>>      visit_type_int(v, NULL, &res, &error_abort);
>>      g_assert_cmpint(res, ==, value);
>> --
>> 2.7.4
>>
>>
> 
> This part is fine I guess.

If desired, I can respin this patch to _just_ the changes under tests/,
as that is all the more that is needed to fix MacOS runtime for 2.8, and
leave the ripping out of %lld for 2.9 for the same time when I rip out
all other % support.  Personally, I found it easier to prove that
nothing was relying on 64-bit parsing by ripping it out completely, so
that glibc platforms would also flag improper uses at runtime, rather
than hoping that I had caught everything by mere grep.  But I guess that
even though 'make check' now passes (and so it appears that we no longer
have any runtime dependency on 64-bit values), going with the more
conservative approach of just fixing the two tests that relied on 64-bit
values but leaving existing support in will avoid problems on glibc and
mingw (but not MacOS) if we still managed to miss something not covered
by 'make check'.

On the bright side, mere grep shows that we really only have 3 remaining
clients of qobject_from_jsonf() in the main code body, none of which use
64-bit ints; and that the only clients of qobject_from_jsonv() are in
the testsuite; so the fact that 'make check' passes is a pretty good
indication that I did not manage to miss anything that still wants to
use dynamic JSON with a 64-bit int.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qapi: Drop support for qobject_from_jsonf("%"PRId64)

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/23/2016 03:25 AM, Paolo Bonzini wrote:
>>> 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, we have already
>>> converted almost all clients to use an alternative method;
>>> convert or eliminate the remaining uses in the testsuite, and
>>> rip out this code from the parser.
>>>
>>> Ripping it all out means that we will now uniformly get
>>> failures on all platforms that try to use dynamic JSON with
>>> 64-bit numbers. Ultimately, I plan for later patches to rip
>>> out dynamic JSON altogether, but that is more invasive and
>>> therefore not appropriate for the 2.8 release, while this
>>> patch fixes an actual testsuite failure of check-qjson on
>>> Mac OS.
>>>
>>> Reported by: G 3 <programmingkidx@gmail.com>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>

The first two patches are bug fixes, and as such they should be
considered for 2.8.

This patch doesn't fix anything, and it might conceivably break
something.  Too late for 2.8.

>> This is throwing out the baby with the bathwater.  %lld works
>> just fine for long long.  Throwing away %I64d is fine though...
>
> On 64-bit systems where int64_t is 'long' rather than 'long long',
> PRId64 is %ld, not %lld.  And on 32-bit MacOS, PRId64 is %qd, which is
> NOT covered by the existing JSON parser.  If you write:
>
> int64_t foo;
> qobject_from_jsonf("%lld", foo)
>
> then gcc will complain on all platforms where %lld is not the right
> string to match against int64_t, thanks to -Wformat.  We could instead
> write:
>
> int64_t foo;
> qobject_from_jsonf("%lld", (long long) foo);
>
> and have that work everywhere, but then you have to explicitly cast.
> And our current qdict_get_int() code returns int64_t, not long long, so
> it's hard to convince people to write:
>
> long long foo;
> foo = qdict_get_int(...);
> qobject_from_jsonf("%lld", foo);
>
> since 'long long' is more typing than 'int64_t'. I suppose we could do
> that, but my next question is why bother.  As I've proven in these three
> patches, there were VERY FEW clients that were trying to use dynamic
> JSON on a 64-bit value in the first place.  Ripping out ALL 64-bit
> support _proves_ that we can't mess it up in the future, vs. leaving
> %lld there and getting it right for some versions of glibc but still
> failing on other platforms when someone uses PRId64 instead of an
> explicit %lld.
>
> My other argument is that I _do_ intend to rip out ALL of the dynamic
> JSON support, at which point we no longer have %d, let along %lld.
> Until you see that followup series and decide whether it was too
> invasive for 2.9, it's hard to say that we are throwing out anything
> useful in this short-term fix for 2.8.  So I guess that gives me a
> reason to hurry up and finish my work on that series to post it today
> before I take a long holiday weekend.

If we rip out _jsonf() in 2.9, then ripping out currently unused parts
of it in 2.8 during hard freeze is needless churn at a rather
inconvenient time.

If we decice not to rip it out, it may well have to be reverted.

I don't think there's a need to hurry, as this patch isn't appropriate
for 2.8 anyway, so there's no reason to quickly decide what to do with
the followup series now.

>>> +++ b/tests/test-qobject-input-visitor.c
>>> @@ -83,10 +83,11 @@ static Visitor
>>> *visitor_input_test_init_raw(TestInputVisitorData *data,
>>>  static void test_visitor_in_int(TestInputVisitorData *data,
>>>                                  const void *unused)
>>>  {
>>> -    int64_t res = 0, value = -42;
>>> +    int64_t res = 0;
>>> +    int value = -42;
>>>      Visitor *v;
>>>
>>> -    v = visitor_input_test_init(data, "%" PRId64, value);
>>> +    v = visitor_input_test_init(data, "%d", value);
>>>
>>>      visit_type_int(v, NULL, &res, &error_abort);
>>>      g_assert_cmpint(res, ==, value);
>>> --
>>> 2.7.4
>>>
>>>
>> 
>> This part is fine I guess.
>
> If desired, I can respin this patch to _just_ the changes under tests/,
> as that is all the more that is needed to fix MacOS runtime for 2.8, and
> leave the ripping out of %lld for 2.9 for the same time when I rip out
> all other % support.  Personally, I found it easier to prove that
> nothing was relying on 64-bit parsing by ripping it out completely, so
> that glibc platforms would also flag improper uses at runtime, rather
> than hoping that I had caught everything by mere grep.  But I guess that
> even though 'make check' now passes (and so it appears that we no longer
> have any runtime dependency on 64-bit values), going with the more
> conservative approach of just fixing the two tests that relied on 64-bit
> values but leaving existing support in will avoid problems on glibc and
> mingw (but not MacOS) if we still managed to miss something not covered
> by 'make check'.

It's a valuable sanity check, but that doesn't mean we should commit it
for 2.8.

> On the bright side, mere grep shows that we really only have 3 remaining
> clients of qobject_from_jsonf() in the main code body, none of which use
> 64-bit ints; and that the only clients of qobject_from_jsonv() are in
> the testsuite; so the fact that 'make check' passes is a pretty good
> indication that I did not manage to miss anything that still wants to
> use dynamic JSON with a 64-bit int.

I'd be willing to buy that, but not this late in hard freeze.

Re: [Qemu-devel] [PATCH 3/3] qapi: Drop support for qobject_from_jsonf("%"PRId64)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

On 11/23/2016 08:17 AM, Markus Armbruster wrote:

> 
> The first two patches are bug fixes, and as such they should be
> considered for 2.8.
> 
> This patch doesn't fix anything, and it might conceivably break
> something.  Too late for 2.8.

Ah, but it DOES fix check-qjson on Mac OS.

As mentioned to Paolo, I'm splitting this into two parts for the v2
series (the first part to fix testsuite failures on Mac OS which is
still 2.8 material, the second to rip out %I64d which becomes more of
2.9 material).

>> My other argument is that I _do_ intend to rip out ALL of the dynamic
>> JSON support, at which point we no longer have %d, let along %lld.
>> Until you see that followup series and decide whether it was too
>> invasive for 2.9, it's hard to say that we are throwing out anything
>> useful in this short-term fix for 2.8.  So I guess that gives me a
>> reason to hurry up and finish my work on that series to post it today
>> before I take a long holiday weekend.
> 
> If we rip out _jsonf() in 2.9, then ripping out currently unused parts
> of it in 2.8 during hard freeze is needless churn at a rather
> inconvenient time.
> 
> If we decice not to rip it out, it may well have to be reverted.
> 
> I don't think there's a need to hurry, as this patch isn't appropriate
> for 2.8 anyway, so there's no reason to quickly decide what to do with
> the followup series now.

Fair enough.

v2 coming soon.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qapi: Drop support for qobject_from_jsonf("%"PRId64)

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/23/2016 08:17 AM, Markus Armbruster wrote:
>
>> 
>> The first two patches are bug fixes, and as such they should be
>> considered for 2.8.
>> 
>> This patch doesn't fix anything, and it might conceivably break
>> something.  Too late for 2.8.
>
> Ah, but it DOES fix check-qjson on Mac OS.

PATCH 1+2 do, don't they?

> As mentioned to Paolo, I'm splitting this into two parts for the v2
> series (the first part to fix testsuite failures on Mac OS which is
> still 2.8 material, the second to rip out %I64d which becomes more of
> 2.9 material).
>
>>> My other argument is that I _do_ intend to rip out ALL of the dynamic
>>> JSON support, at which point we no longer have %d, let along %lld.
>>> Until you see that followup series and decide whether it was too
>>> invasive for 2.9, it's hard to say that we are throwing out anything
>>> useful in this short-term fix for 2.8.  So I guess that gives me a
>>> reason to hurry up and finish my work on that series to post it today
>>> before I take a long holiday weekend.
>> 
>> If we rip out _jsonf() in 2.9, then ripping out currently unused parts
>> of it in 2.8 during hard freeze is needless churn at a rather
>> inconvenient time.
>> 
>> If we decice not to rip it out, it may well have to be reverted.
>> 
>> I don't think there's a need to hurry, as this patch isn't appropriate
>> for 2.8 anyway, so there's no reason to quickly decide what to do with
>> the followup series now.
>
> Fair enough.
>
> v2 coming soon.

Thanks!

Re: [Qemu-devel] [PATCH 3/3] qapi: Drop support for qobject_from_jsonf("%"PRId64)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

On 11/23/2016 10:56 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/23/2016 08:17 AM, Markus Armbruster wrote:
>>
>>>
>>> The first two patches are bug fixes, and as such they should be
>>> considered for 2.8.
>>>
>>> This patch doesn't fix anything, and it might conceivably break
>>> something.  Too late for 2.8.
>>
>> Ah, but it DOES fix check-qjson on Mac OS.
> 
> PATCH 1+2 do, don't they?

No, patch 1 fixes emission of QMP events, patch 2 fixes check-qga. Also
reported broken by G 3 was check-qjson, and my audit revealed that
test-qobject-input-visitor is also affected.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]