Re: [Qemu-devel] [PATCH 4/9] tests: Clean up string interpolation into QMP input (simple cases)

[Eric Blake]

On 07/21/2017 11:48 AM, Markus Armbruster wrote:
> I forgot that PRId64 expands into non-standard crap on some systems.
> 
> Options:
> 
> 1. Outlaw use of PRI macros, limit integer length modifiers for
>    conversion specifiers "d" and "u" to "l" and "ll".  When PRI macros
>    creep in, the build breaks on hosts where they expand to anything
>    else (hopefully, our CI will catch that).  Interpolating int64_t
>    values become a bit awkward.
> 
>    Required work: fix my patches not to use PRId64, drop support for
>    length modifier "I64" from parse_escape().

Yuck.  (But motivation for my earlier series to nuke qobject_from_jsonf())

> 
> 2. Support PRId64 and PRIu64, whatever their actual value may be.
> 
>    a. Support all possible values.  This is what we've tried before.
>       Nasty: fails only at run-time on hosts with sufficiently creative
>       values.
> 
>       Required work: add support for length modifier "q", to unbreak
>       MacOS.
> 
>       Optional work: try to turn the run-time failure into a compile-
>       time failure, ideally in configure.

Accepts garbage (although -Wformat detection will help avoid the worst
of it).  You can't check string equality in the preprocessor, and you
can't do things like "case PRId64[0]: case 'l':" to force compilation
errors due to duplicate labels, but we CAN limit ourselves to the
preprocessor and grep that output to see what PRId64 expands to.  I'll
see if I can come up with a configure check.  Still, it may be easier to
just fail configure and tell people to report the bug on their odd libc,
at which point we update json-lexer.c and configure, than it is to try
and reuse configure results in json-lexer.c (since the PRId64 string is
not a constant width, it gets hard to code an exact value into our lexer
state machine, but easier to code every reported value).

> 
>    b. Support exactly the host's PRId64 and PRIu64 values.
> 
>       Work required: replace support of "I64d" and "I64u" by support of
>       PRId64 and PRIu64.  Easy enough in parse_escape(), but not in
>       json-lexer.c.  We could perhaps have the lexer accept the shortest
>       string that's followed by a conversion specifier as length
>       modifier, then reject invalid length modifiers in a semantic
>       action.

Interesting idea. I'm playing with it...

> 
>    Optional work: try to simplify code that currently works around
>    unavailability of PRId64 and PRIu64.
> 
> Preferences?
> 
> I like 2b, but I'm not sure I'll like the code implementing it.  One way
> to find out...

In the lexer, widen the state machine to accept up to three unknown
characters between % and d.  Hmm, there's also the possibility of
int64_t being mapped to %jd, in addition to our known culprits of %ld,
%lld, %I64d, and %qd.

> 
>> Overall, I like the patch, but we need to fix the problems for the next
>> round of this series.
> 
> Yes.  

At this point, I think I'll spin the next round. But it's not longer a
2.10 priority, so it may be a few days.

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

signature.asc
Description: OpenPGP digital signature