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


> 
> 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 \").

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org