| Message ID | 1479874588-1969-4-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
> 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
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> 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.
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> 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!
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.
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);
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(-)