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

[Eric Blake]

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 <address@hidden>
>> Signed-off-by: Eric Blake <address@hidden>
> 
> 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

signature.asc
Description: OpenPGP digital signature