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

[Eric Blake]

On 11/23/2016 03:23 AM, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
>> From: "Eric Blake" <address@hidden>
>> To: address@hidden
>> Cc: address@hidden, address@hidden, address@hidden
>> Sent: Wednesday, November 23, 2016 5:16:27 AM
>> Subject: [PATCH 2/3] test-qga: Avoid qobject_from_jsonf("%"PRId64)
>>
>> The qobject_from_jsonf() function implements a pseudo-printf
>> language for creating a QObject; however, it is hard-coded to
>> only parse a subset of formats understood by printf().  In
>> particular, any use of a 64-bit integer works only if the
>> system's definition of PRId64 matches what the parser expects;
>> which works on glibc (%lld) and mingw (%I64d), but not on
>> Mac OS (%qd).  Rather than enhance the parser, it is just as
>> easy to use normal printf() for this particular conversion,
>> matching what is done elsewhere in this file.

This is the key, look at:
$ git grep -A2 strdup_printf tests/test-qga.c

and you'll see that my change is no grosser than the rest of the file.

>> -        ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
>> -                     " 'arguments': { 'pid': %" PRId64 "  } }", pid);
>> +        char *cmd;
>> +
>> +        cmd = g_strdup_printf("{'execute': 'guest-exec-status',"
>> +                              " 'arguments': { 'pid': %" PRId64 " } }",
>> +                              pid);

_THIS_ patch merely moves the (pre-existing, ugly) association of
"%"PRId64, pid from qobject_from_jsonf() to g_strdup_printf().  But your
question...

> 
> This is too ugly to see. :)  Why not use %lld, or just make pid an
> int?  Are there really systems with 64-bit pid_t?

...is whether the pre-existing code is correct.  For the purposes of
fixing Mac OS compilation, I argue that your question doesn't matter.  I
agree with you that the code looks gross, but that's not my fault.  But
here's why I think changing the ugliness is wrong:

The existing code already used 64-bit pid (note, this is 'pid', not
'pid_t'), because of:

    int64_t pid, now, exitcode;
...
    val = qdict_get_qdict(ret, "return");
    pid = qdict_get_int(val, "pid");
    g_assert_cmpint(pid, >, 0);

And yes, the qapi schema currently lists a 64-bit type (anything without
an explicit size is 64 bits over JSON):

{ 'struct': 'GuestExec',
  'data': { 'pid': 'int'} }

And yes, 64-bit mingw has a 64-bit pid_t (ewwww)

/usr/x86_64-w64-mingw32/sys-root/mingw/include/sys/types.h:
#ifndef _PID_T_
#define _PID_T_
#ifndef _WIN64
typedef int     _pid_t;
#else
__MINGW_EXTENSION
typedef __int64 _pid_t;
#endif

although there is a long-standing bug in mingw headers, since getpid()
returns a different type than pid_t:

/usr/x86_64-w64-mingw32/sys-root/mingw/include/unistd.h:
  int __cdecl getpid(void) __MINGW_ATTRIB_DEPRECATED_MSVC2005;

and sadly, mingw lacks the POSIX-required id_t type (which must be at
least as large as any of pid_t, uid_t, or gid_t) to know if it was
intentional.  At any rate, I worry that changing the QGA definition to
specify 'int32' may break compilation on mingw, where the qga code would
have to start casting from the (possibly-broken) 64-bit pid_t down to a
32-bit value when populating the QAPI struct to pass back over the wire.

I _really_ wish mingw would fix their headers (decide if 64-bit pid_t is
really correct or not, and then make pid_t and getpid() match as well as
supply id_t), but this isn't the forum to get that accomplished.

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

signature.asc
Description: OpenPGP digital signature