Re: [Qemu-devel] [PATCH] qobject: Accept "%"PRId64 in qobject_from_jsonf()

[Eric Blake]

On 07/24/2017 04:06 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Commit 1792d7d0 was written because PRId64 expands to non-portable
>> crap for some libc, and we had testsuite failures on Mac OS as a
>> result.  This in turn makes it difficult to rely on the obvious
>> conversions of 64-bit values into JSON, requiring things such as
>> casting int64_t to long long so we can use a reliable %lld and
>> still keep -Wformat happy.  So now it's time to fix that.
>>

>> +case $(strings $TMPE | grep ^UnLiKeLyToClAsH) in
>> +    '' | *"$nl"* ) error_exit "can't determine value of PRId64" ;;
>> +    *_ld | *_lld | *_I64d | *_qd) ;;
>> +    *) error_exit "unexepected value of PRId64, please add %$(strings $TMPE 
>> |
>> +       sed -n s/^UnLiKeLyToClAsH_//p) support to json-lexer.c" ;;
>> +esac
>> +
> 
> Why is this easier or more robust than examining output of the
> preprocessor?  Hmm, you explain it in the commit message.  I think you
> should also (briefly!) explain it in the "Sadly" comment.

Okay.  (Something along the lines of: We can't guarantee if the
preprocessor will produce "ld" or "l" "d", nor even if the expansion
will occur on the same line as any marker)

I also wonder if I should anchor some \n in the magic bytes being
searched for in the binary, so that if 'strings' fails (which may indeed
be the case for a mingw binary), then falling back to raw grep may also
have a chance.  But first, I'm hoping to get some patchew feedback first
if one of the build platforms has problems with the current attempt.


>> +++ b/qobject/json-lexer.c
>> @@ -31,7 +31,7 @@
>>   *
>>   * Extension for vararg handling in JSON construction:
>>   *
>> - * %((l|ll|I64)?d|[ipsf])
>> + * %(PRI[du]64|(l|ll)?[ud]|[ipsf])
> 
> Confusing.  The lexer accepts more than that, but parse_escape() filters
> it out.  Need a comment explaining what, because the latter isn't
> locally obvious.

True - we lex all known forms, and then only parse the current
platform's form.  Will improve the comment.


>>      } else if (!strcmp(token->str, "%ld")) {
>>          return QOBJECT(qnum_from_int(va_arg(*ap, long)));
>> -    } else if (!strcmp(token->str, "%lld") ||
>> -               !strcmp(token->str, "%I64d")) {
>> +    } else if (!strcmp(token->str, "%" PRId64)) {
>> +        return QOBJECT(qnum_from_int(va_arg(*ap, int64_t)));
>> +    } else if (!strcmp(token->str, "%lld")) {
>>          return QOBJECT(qnum_from_int(va_arg(*ap, long long)));
> 
> Let's do "ll" before PRId64.

Sure.


>> +++ b/tests/check-qjson.c
>> @@ -990,8 +990,10 @@ static void vararg_number(void)
>>      QNum *qnum;
>>      int value = 0x2342;
>>      long long value_ll = 0x2342342343LL;
>> +    uint64_t value_u = UINT64_C(0x2342342343);
> 
> Is UINT64_C() really necessary here?

Not as long as none of the compilers we use complains about uint64_t x =
1ULL.  I'll simplify, then we can uglify if a compiler complains.

> 
> Call the variable value_u64?

Yes.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature