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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

Yes.

> 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.

The type of plain 0x2342342343 is the first of int, unsigned int, long
int, unsigned long int, long long int, unsigned long long int that can
represent it.  In practice, either long or long long.  Can't see
anything for compilers to whine about there, but I've been wrong before.

>> Call the variable value_u64?
>
> Yes.