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

[Markus Armbruster]

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

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.