Re: [Qemu-devel] [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 11/23/2016 08:05 AM, Markus Armbruster wrote:
>
>> Same problem as in the previous patch, but here you replace it by
>> g_strdup_printf(), where the previous patch replaced it by manual
>> QObject construction,
>> 
>> Manual QObject construction tends to be less readable.
>
> Are there things we can do to make it more readable to the point where
> it would be tolerable in the situations where it is needed?
>
> One of the patches on my dynamic-JSON removal series adds a new:
>
> qdict_put_int(dict, "key", 1);
>
> which is a lot more legible than:
>
> qdict_put(dict, "key", qint_from_int(1));

It's more legible, but I wouldn't call it "a lot more legible".

>> g_strdup_printf() doesn't have that problem, but it has a more serious
>> one: escaping for JSON is no longer below the hood.
>> 
>> Since the string gets passed to qmp_fd(), we additionally need to escape
>> '%'.
>
> Worse, the escaping of %s differs between the two (in printf, %s just
> concatenates strings, in dynamic JSON, it adds outer "" and escapes
> inner " into \").

That's a feature.  It actually escapes much more than just '"'.  Have a
look at to_json() case QTYPE_QSTRING.

The imporant bit here is: _jsonf() is not printf()!  The part it shares
with printf() is the argument types associated with conversion
specifiers, and it shares them just because that way the compiler can
help us catch type errors.  What it does with the arguments is
*different*, because what it does is different.  It does *not* format a
string.  Not even conceptually.  It builds a QObject from a string
template.

>> Interfaces that require callers to escape almost inevitably result in
>> bugs if experience is any guide.  Safer, less low level interfaces are
>> preferable.
>> 
>> Nothing actually needs escaping here, so your code isn't wrong.  It's
>> just a bad example.
>> 
>> You've pointed out that the file is chock-full of bad examples already,
>> so one more won't make a difference.  Point taken regarding the
>> immediate fix.  But I doubt it a sane strategy for replacing _jsonf().
>
> Well, until I post my conversion series that eliminates _json[fv](), we
> don't have any hard numbers on how many bad examples remain, or whether
> the cleanup looks worth it.

Yes.  Without patches, the discussion is speculative.