From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:57246)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1WTS30-0003L9-1d
	for qemu-devel@nongnu.org; Fri, 28 Mar 2014 04:22:00 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1WTS2s-0007Hq-5v
	for qemu-devel@nongnu.org; Fri, 28 Mar 2014 04:21:53 -0400
Received: from mx1.redhat.com ([209.132.183.28]:45023)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1WTS2r-0007Hk-SO
	for qemu-devel@nongnu.org; Fri, 28 Mar 2014 04:21:46 -0400
From: Markus Armbruster <armbru@redhat.com>
References: <1395907390-18812-1-git-send-email-wenchaoqemu@gmail.com>
	<1395907390-18812-3-git-send-email-wenchaoqemu@gmail.com>
	<5334B023.5060602@redhat.com>
Date: Fri, 28 Mar 2014 09:21:40 +0100
In-Reply-To: <5334B023.5060602@redhat.com> (Eric Blake's message of "Thu, 27
	Mar 2014 17:11:31 -0600")
Message-ID: <87ppl691ij.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [RFC PATCH V4 2/5] qapi: add event helper functions
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Eric Blake <eblake@redhat.com>
Cc: mdroth@linux.vnet.ibm.com, lcapitulino@redhat.com, Wenchao Xia <wenchaoqemu@gmail.com>, qemu-devel@nongnu.org

Eric Blake <eblake@redhat.com> writes:

> On 03/27/2014 02:03 AM, Wenchao Xia wrote:
>> This file holds some functions that do not need to be generated.
>> 
>> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
>> ---
>>  include/qapi/qmp-event.h |   27 +++++++++++++++++
>>  qapi/Makefile.objs       |    1 +
>>  qapi/qmp-event.c         |   70 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 98 insertions(+), 0 deletions(-)
>>  create mode 100644 include/qapi/qmp-event.h
>>  create mode 100644 qapi/qmp-event.c
>> 
>
>> +    err = qemu_gettimeofday(&tv);
>> +    if (err < 0) {
>> +        /* Put -1 to indicate failure of getting host time */
>> +        tv.tv_sec = -1;
>> +        tv.tv_usec = -1;
>
> You fixed the problem with C promotion here, but...
>
>> +    }
>> +
>> +    obj = qobject_from_jsonf("{ 'seconds': %" PRId64 ", "
>> +                             "'microseconds': %" PRId64 " }",
>> +                             (int64_t) tv.tv_sec, (int64_t) tv.tv_usec);
>
> ...here, C promotion rules bite once again :(  If tv_usec is uint32_t,
> then it zero-extends rather than sign-extends into int64_t, and you may
> end up with 0xffffffff instead of the intended -1.  When doing
> potentially widening casts, it is only safe if you know the signedness
> of the pre-cast value; but with struct timeval, POSIX doesn't make that
> easy.
>
> Maybe it's easier to just rewrite things with known types:
>
> int64_t sec;
> int usec;
> qemu_timval tv;
> err = qemu_gettimeofday(&tv);
> if (err < 0) {
>     sec = -1;
>     usec = -1;
> } else {
>     sec = tv.tv_sec;
>     usec = tv.tv_usec;
> }
> qobject_from_jsonf("... %"PRId64 ", ...%d", sec, usec)
>
> since 'int' is guaranteed to be large enough for all the usec values we
> care about on all platforms we compile on (that is, we require 32-bit
> int, even if C allows for a 16-bit int implementation).

If you go that route, then why not go all the way nad make both sec and
usec int64_t?  Look ma, no type casts!