From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:38474) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RhRTD-0008UF-6A for qemu-devel@nongnu.org; Sun, 01 Jan 2012 14:53:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RhRTC-00035E-Ce for qemu-devel@nongnu.org; Sun, 01 Jan 2012 14:53:27 -0500 Received: from mail-ee0-f45.google.com ([74.125.83.45]:57292) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RhRTC-00035A-2X for qemu-devel@nongnu.org; Sun, 01 Jan 2012 14:53:26 -0500 Received: by eekb45 with SMTP id b45so17502332eek.4 for ; Sun, 01 Jan 2012 11:53:24 -0800 (PST) Sender: Paolo Bonzini Message-ID: <4F00B9B1.5030300@redhat.com> Date: Sun, 01 Jan 2012 20:53:21 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1321898431-18449-1-git-send-email-pbonzini@redhat.com> <1321898431-18449-2-git-send-email-pbonzini@redhat.com> <4F00734D.8020008@redhat.com> In-Reply-To: <4F00734D.8020008@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/4] rtc: fix 12-hour mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: qemu-devel@nongnu.org On 01/01/2012 03:53 PM, Avi Kivity wrote: > On 11/21/2011 08:00 PM, Paolo Bonzini wrote: >> Hours in 12-hour mode are in the 1-12 range, not 0-11. >> >> @@ -320,7 +324,8 @@ static void rtc_copy_date(RTCState *s) >> s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour); >> } else { >> /* 12 hour format */ >> - s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour % 12); >> + int h = (tm->tm_hour % 12) ? tm->tm_hour % 12 : 12; >> + s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, h); >> if (tm->tm_hour>= 12) >> s->cmos_data[RTC_HOURS] |= 0x80; >> } > > Nitpick, don't update patch on this account: > > I dislike seeing int-to-bool conversion on anything that is not a > true/false or count value. Things like if (has_some_property) or if > (!nr_items) read well and are easily understood. But here, you're not > checking for "are there any tm_hours, if yes, use them, if not, use > 12". You're testing against the value 0 which has a special encoding in > 12 hour mode. > > The is usually manifested in > > if (!strcmp(a, b)) ... > > strcmp() does not return a bool or a count, and in fact it reads exactly > the opposite of the intent: "if not string compare". strcmp() returns > an enumeration, or perhaps a mapping of string trichotomy to integer > trichotomy. > > Sorry about the pontification, back to the regularly scheduled > whitespace discussion. Yeah, I probably would have remarked the same if it was someone else's patch. :) BTW, I have kvm-unit-tests tests for these and other RTC emulation problems. Long term perhaps they should become qtests, but for now kvm-unit-tests is the best place. However, without these four patches they hang which is a bit too heavy. So I'll submit as soon as these are in. Paolo