Patchwork Fix conversion between 12 hours and 24 hours modes.

login
register
mail settings
Submitter Antoine Mathys
Date Dec. 14, 2012, 10:53 p.m.
Message ID <1355525607-2398-1-git-send-email-barsamin@gmail.com>
Download mbox | patch
Permalink /patch/206589/
State New
Headers show

Comments

Antoine Mathys - Dec. 14, 2012, 10:53 p.m.
The proper mapping between 24 hours and 12 hours modes is:
0	12 AM
1-11	1-11 AM
12	12 PM
13-23	1-11 PM
Fix code accordingly.

Signed-off-by: Antoine Mathys <barsamin@gmail.com>
---
 hw/ds1338.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Peter Maydell - Feb. 14, 2013, 11:11 a.m.
On 14 December 2012 22:53, Antoine Mathys <barsamin@gmail.com> wrote:
> The proper mapping between 24 hours and 12 hours modes is:
> 0       12 AM
> 1-11    1-11 AM
> 12      12 PM
> 13-23   1-11 PM
> Fix code accordingly.
>
> Signed-off-by: Antoine Mathys <barsamin@gmail.com>

Sorry, I missed this patch earlier.
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(and I've had confirmation from somebody that this change
makes us match the hardware behaviour where AM/PM is
toggled when we go from 11:59 to 12:00).

I tweaked the commit message a little (added hw/ds1338:
prefix to the summary and removed the hardcoded tabs)
and have queued it in arm-devs.next.

thanks
-- PMM
Andreas Färber - Feb. 14, 2013, 11:29 a.m.
Am 14.02.2013 12:11, schrieb Peter Maydell:
> On 14 December 2012 22:53, Antoine Mathys <barsamin@gmail.com> wrote:
>> The proper mapping between 24 hours and 12 hours modes is:
>> 0       12 AM
>> 1-11    1-11 AM
>> 12      12 PM
>> 13-23   1-11 PM
>> Fix code accordingly.
>>
>> Signed-off-by: Antoine Mathys <barsamin@gmail.com>
> 
> Sorry, I missed this patch earlier.
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> (and I've had confirmation from somebody that this change
> makes us match the hardware behaviour where AM/PM is
> toggled when we go from 11:59 to 12:00).
> 
> I tweaked the commit message a little (added hw/ds1338:
> prefix to the summary and removed the hardcoded tabs)
> and have queued it in arm-devs.next.

I may be repeating myself here, but adding qtests as requested would've
caught this. And they should even more be added to avoid regressions.
There's even templates to copy for RTC, and the new Big Endian-safe
{read,write}[bwlq]() functions for 1.5 are on the list and got a
Reviewed-by from Anthony in the unpolished version, so only
implementation details might still change.

Regards,
Andreas
Peter Maydell - Feb. 14, 2013, 11:32 a.m.
On 14 February 2013 11:29, Andreas Färber <afaerber@suse.de> wrote:
> Am 14.02.2013 12:11, schrieb Peter Maydell:
>> On 14 December 2012 22:53, Antoine Mathys <barsamin@gmail.com> wrote:
>>> The proper mapping between 24 hours and 12 hours modes is:
[snip]

>> (and I've had confirmation from somebody that this change
>> makes us match the hardware behaviour where AM/PM is
>> toggled when we go from 11:59 to 12:00).

> I may be repeating myself here, but adding qtests as requested would've
> caught this.

Er, only if the tests were testing for the right thing,
which seems unlikely since you can't run a qtest against
real hardware. What seems more likely is that we would
have written a qtest which checked for the same wrong
behaviour we incorrectly put into the code, which
doesn't help anybody.

-- PMM
Andreas Färber - Feb. 14, 2013, 11:44 a.m.
Am 14.02.2013 12:32, schrieb Peter Maydell:
> On 14 February 2013 11:29, Andreas Färber <afaerber@suse.de> wrote:
>> Am 14.02.2013 12:11, schrieb Peter Maydell:
>>> On 14 December 2012 22:53, Antoine Mathys <barsamin@gmail.com> wrote:
>>>> The proper mapping between 24 hours and 12 hours modes is:
> [snip]
> 
>>> (and I've had confirmation from somebody that this change
>>> makes us match the hardware behaviour where AM/PM is
>>> toggled when we go from 11:59 to 12:00).
> 
>> I may be repeating myself here, but adding qtests as requested would've
>> caught this.
> 
> Er, only if the tests were testing for the right thing,
> which seems unlikely since you can't run a qtest against
> real hardware. What seems more likely is that we would
> have written a qtest which checked for the same wrong
> behaviour we incorrectly put into the code, which
> doesn't help anybody.

Still my point is we should stop accepting RTC bugfixes without test
cases or we'll never get things testable... Paolo and me can't write the
qtests for everyone, that doesn't scale.

In particular in this case the same author is touching on the other RTC
as well and refusing to supply a test case for the pre-existing
rtc-test, which is not nice.

And yes, it's always possible a test case is wrong, then it notices a
change in behavior by starting to fail and can be updated along with the
change. Remember, maintainers are supposed to run `make check`. ;)

Andreas
Peter Maydell - Feb. 14, 2013, 11:49 a.m.
On 14 February 2013 11:44, Andreas Färber <afaerber@suse.de> wrote:
> Am 14.02.2013 12:32, schrieb Peter Maydell:
>> On 14 February 2013 11:29, Andreas Färber <afaerber@suse.de> wrote:
>>> I may be repeating myself here, but adding qtests as requested would've
>>> caught this.
>>
>> Er, only if the tests were testing for the right thing,
>> which seems unlikely since you can't run a qtest against
>> real hardware. What seems more likely is that we would
>> have written a qtest which checked for the same wrong
>> behaviour we incorrectly put into the code, which
>> doesn't help anybody.
>
> Still my point is we should stop accepting RTC bugfixes without test
> cases or we'll never get things testable... Paolo and me can't write the
> qtests for everyone, that doesn't scale.

I'm not saying "test cases are bad", I'm just saying I think
you're wrong when you claim "test cases would have made us
notice the need for this bugfix".

-- PMM
Paolo Bonzini - Feb. 14, 2013, 12:17 p.m.
Il 14/02/2013 12:44, Andreas Färber ha scritto:
> What seems more likely is that we would
> have written a qtest which checked for the same wrong
> behaviour we incorrectly put into the code, which
> doesn't help anybody.

Hmm, that's not how I write tests...  You write them as black boxes, and
if things don't match what you expect, you either replicate the qtest on
real hardware or check the datasheet.

Paolo
Paolo Bonzini - Feb. 14, 2013, 12:17 p.m.
Il 14/02/2013 12:29, Andreas Färber ha scritto:
> I may be repeating myself here, but adding qtests as requested would've
> caught this. And they should even more be added to avoid regressions.
> There's even templates to copy for RTC, and the new Big Endian-safe
> {read,write}[bwlq]() functions for 1.5 are on the list and got a
> Reviewed-by from Anthony in the unpolished version, so only
> implementation details might still change.

When this patch was posted, the i2c libqos was not committed yet.  Also,
the rules we gave ourselves at QEMU Summit (BTW where are the minutes?
still on etherpad only?) are that qtests will be needed only for new
devices.  So it's left to the good will of the maintainers to add new
qtests for old devices.

Paolo
Peter Maydell - Feb. 14, 2013, 12:22 p.m.
On 14 February 2013 12:17, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 14/02/2013 12:44, Andreas Färber ha scritto:
>> What seems more likely is that we would
>> have written a qtest which checked for the same wrong
>> behaviour we incorrectly put into the code, which
>> doesn't help anybody.
>
> Hmm, that's not how I write tests...  You write them as black boxes, and
> if things don't match what you expect, you either replicate the qtest on
> real hardware or check the datasheet.

In this case the data sheet was ambiguous. The only way
to avoid the bug would be to examine actual hardware
behaviour, or to have different people write the test and
the code. If the patch author had written the test case
they'd just have resolved the ambiguity in the same (wrong)
way in both code and test.

-- PMM
Andreas Färber - Feb. 14, 2013, 12:26 p.m.
Am 14.02.2013 13:17, schrieb Paolo Bonzini:
> Il 14/02/2013 12:44, Andreas Färber ha scritto:
>> What seems more likely is that we would
>> have written a qtest which checked for the same wrong
>> behaviour we incorrectly put into the code, which
>> doesn't help anybody.
> 
> Hmm, that's not how I write tests...  You write them as black boxes, and
> if things don't match what you expect, you either replicate the qtest on
> real hardware or check the datasheet.

We seem in violent agreement once again. :)
My point was, if there's an error in the device, then either the test
case had a thinko or there's no test case covering that particular code
path (which I was just lobbying against). You're saying, if one writes
one's test cases right, then there's no bugs in those code paths. True.

Andreas
Antoine Mathys - Feb. 15, 2013, 2:49 a.m.
First, the ds1338 code was in a poor state and never handled the 12 hour 
clock correctly. My first patch failed to fully fix the problem so I had 
to write a second one, but at no point did Peter or I introduce a 
regression, quite the opposite.

Second, I don't know where you got the idea that I refuse to write test 
cases. I just didn't have one ready or in the works at the time.

Third, bug 1090558 in mc146818rtc is a good example of a bug which was 
not due to insufficient testing, but to poorly structured code.

There is no point worrying about unit testing if you accept code of such 
low quality. This goes for the tests too. For instance 
cmos_get_date_time() in tests/rtc-test.c doesn't work correctly in 12 
hour mode.

Fourth, I am not interested in the PC architecture, I only wrote a fix 
for bug 1090558 because Paolo asked me to. It is nice to see that fixing 
your crappy code makes me "not a nice guy" who is making things worse. 
But don't worry, I'll focus on ARM from now on.
Paolo Bonzini - Feb. 15, 2013, 8:32 a.m.
Il 15/02/2013 03:49, Antoine Mathys ha scritto:
> First, the ds1338 code was in a poor state and never handled the 12 hour
> clock correctly. My first patch failed to fully fix the problem so I had
> to write a second one, but at no point did Peter or I introduce a
> regression, quite the opposite.
> 
> Second, I don't know where you got the idea that I refuse to write test
> cases. I just didn't have one ready or in the works at the time.
> 
> Third, bug 1090558 in mc146818rtc is a good example of a bug which was
> not due to insufficient testing, but to poorly structured code.
> 
> There is no point worrying about unit testing if you accept code of such
> low quality. This goes for the tests too. For instance
> cmos_get_date_time() in tests/rtc-test.c doesn't work correctly in 12
> hour mode.
> 
> Fourth, I am not interested in the PC architecture, I only wrote a fix
> for bug 1090558 because Paolo asked me to. It is nice to see that fixing
> your crappy code makes me "not a nice guy" who is making things worse.
> But don't worry, I'll focus on ARM from now on.

Hey hey, no reason to get excited.

Yes, some code is of pretty low quality.  We're getting better, the main
problem is that without a testing infrastructure there's only so much
you can do for code quality.  Hence Andreas's request.

Thanks for your PC patch.  Nobody said you're making things worse.

Paolo
Andreas Färber - Feb. 15, 2013, 11:24 a.m.
Antoine,

Am 15.02.2013 03:49, schrieb Antoine Mathys:
> First, the ds1338 code was in a poor state and never handled the 12 hour
> clock correctly. My first patch failed to fully fix the problem so I had
> to write a second one, but at no point did Peter or I introduce a
> regression, quite the opposite.

Read closely, I never claimed *you* introduced a regression. What I have
rather been observing is a seemingly not-ending stream of bugfix patches
on that matter and Peter not making an effort of requesting qtest cases
from you or for any new ARM devices elsewhere.

And while we're at it, what annoys me personally is that this patch does
not have a "ds1338: " prefix when it doesn't touch anything else.
People like me need to go through git logs for potential backports and
having that made unnecessarily hard sucks. Peter can hopefully fix that
in his arm-devs.next queue.

> Second, I don't know where you got the idea that I refuse to write test
> cases. I just didn't have one ready or in the works at the time.

From reading the mailing list, obviously:

https://bugs.launchpad.net/qemu/+bug/1090558

-> closed by Paolo due to lack of test case, no response of yours

http://patchwork.ozlabs.org/patch/220390/

Quote:
>> Do you have a testcase?
> No, but the refactoring makes the code very easy to audit.

The expected answer would've been "take guest X and do Y to see Z", or
better to extend the existing qtest cases to prove something was broken
before and fixed afterwards and to avoid the same bug being reintroduced
later. qtest has special commands to control time fwiw, so it should be
entirely possible to set the time to 0, 12, 23 and anything-in-between
hours to verify the register values are as expected.

> Third, bug 1090558 in mc146818rtc is a good example of a bug which was
> not due to insufficient testing, but to poorly structured code.
> 
> There is no point worrying about unit testing if you accept code of such
> low quality. This goes for the tests too. For instance
> cmos_get_date_time() in tests/rtc-test.c doesn't work correctly in 12
> hour mode.
> 
> Fourth, I am not interested in the PC architecture, I only wrote a fix
> for bug 1090558 because Paolo asked me to. It is nice to see that fixing
> your crappy code makes me "not a nice guy" who is making things worse.
> But don't worry, I'll focus on ARM from now on.

You seem to be missing the point: My comments have exactly nothing to do
with PC vs. ARM or with you "making things worse"! It's about you a)
supplying a bunch of "fixes" without giving maintainers a preferably
automated way to verify they are in fact correct and b) - judging from
your replies - being stubborn in grasping that you are not the only one
supplying patches to QEMU and that while you may understand the code
better now, someone else might not and may well introduce regressions
even if you personally didn't - from a maintenance QA point of view it
makes no difference who introduces a bug.

QEMU has a long history of patch submissions and, as you have noticed on
the topic of I2C, our test infrastructure is still new. The code quality
doesn't get improved by complaining about it being low though but by
improving code (that part you have done) and ensuring that the quality
doesn't regress (that's the part this discussion is about). qtest is the
most convenient infrastructure since it's integrated into make check and
can easily be run by any maintainer or contributor; another option is
the external virt-test framework (which didn't seem to have any
provisions for ARM last time I looked around).

So, my nagging for qtest test cases for ds1338 does not get resolved by
saying you'll focus on ARM now and stay out of PC, because if I am not
completely mistaken ds1338 is all about ARM. IIUC such an I2C device can
be instantiated via -device using the available machine that I added
libqos support for (-M n800 or -M n810), to prove that it is easily
possible. You can't expect me to write everything for you!

Regards,
Andreas
Peter Maydell - Feb. 15, 2013, 11:41 a.m.
On 15 February 2013 11:24, Andreas Färber <afaerber@suse.de> wrote:
> Am 15.02.2013 03:49, schrieb Antoine Mathys:
>> First, the ds1338 code was in a poor state and never handled the 12 hour
>> clock correctly. My first patch failed to fully fix the problem so I had
>> to write a second one, but at no point did Peter or I introduce a
>> regression, quite the opposite.
>
> Read closely, I never claimed *you* introduced a regression. What I have
> rather been observing is a seemingly not-ending stream of bugfix patches

One patch is hardly a never-ending stream!

> on that matter and Peter not making an effort of requesting qtest cases
> from you or for any new ARM devices elsewhere.

If people want to provide test cases, cool; I'm not currently
insisting on them.

> And while we're at it, what annoys me personally is that this patch does
> not have a "ds1338: " prefix when it doesn't touch anything else.
> People like me need to go through git logs for potential backports and
> having that made unnecessarily hard sucks. Peter can hopefully fix that
> in his arm-devs.next queue.

Yes, and I said so in my review-and-accepted mail yesterday.

-- PMM
Antoine Mathys - Feb. 15, 2013, 3:41 p.m.
On 02/15/2013 12:24 PM, Andreas Färber wrote:
> The expected answer would've been "take guest X and do Y to see Z", or
> better to extend the existing qtest cases to prove something was broken
> before and fixed afterwards and to avoid the same bug being reintroduced
> later.

If we are talking about adding a test case in order to have some guarantee that what works after a fix keeps working in the future, that's fine. And I am willing to add such tests for the DS1338 implementation (once it is finished).

But demanding a test case that the code passes with the fix but fails without, in order to prove that something was broken before, is only reasonable if the bug was found through testing in the first place.

It is inappropriate for a bug found in a code review. Not only do you not need a test case to prove the bug exists, but reverse-engineering a test-case can be a significant undertaking. Paolo tried to do that unsuccessfully in the case of bug 1090558 and I had no reason to think I could do better. This does not strike me as a very productive use of developer time anyway.

And your suggestion that it is better to leave a known bug unpatched until someone can conjure up a test case is ridiculous. I don't see how that attitude help users, in the short or long term.

If you don't nuance your position you are only going to discourage much needed code reviews. I don't see what good can come of that.
Paolo Bonzini - Feb. 15, 2013, 4:15 p.m.
Il 15/02/2013 16:41, Antoine Mathys ha scritto:
> On 02/15/2013 12:24 PM, Andreas Färber wrote:
>> The expected answer would've been "take guest X and do Y to see Z", or
>> better to extend the existing qtest cases to prove something was broken
>> before and fixed afterwards and to avoid the same bug being reintroduced
>> later.
> 
> If we are talking about adding a test case in order to have some
> guarantee that what works after a fix keeps working in the future,
> that's fine. And I am willing to add such tests for the DS1338
> implementation (once it is finished).
> 
> But demanding a test case that the code passes with the fix but fails
> without, in order to prove that something was broken before, is only
> reasonable if the bug was found through testing in the first place.

It depends.  For example, the mc146818rtc model was rewritten almost
completely last year.  Testcases helped a lot, and more testcases would
have helped even more.  It does not matter if they came from past bugs,
or from code review, or from blackbox testing.

> Not only do you
> not need a test case to prove the bug exists, but reverse-engineering a
> test-case can be a significant undertaking.

That's true.  But...

> Paolo tried to do that
> unsuccessfully in the case of bug 1090558 and I had no reason to think I
> could do better. This does not strike me as a very productive use of
> developer time anyway.

... at least trying to do that _is_ a productive use of developer time.
 Of course, insisting at all costs is not.  So a reviewer is right in
asking for a testcase and complaining of a lack of testcases.  He should
be okay with "I couldn't find one" just as well, especially if it comes
with a patch that actually adds testcases.  My effort to reproduce bug
1090558 did produce such a patch.

As an aside ds1338 is a much simpler model than mc146818rtc (the
"capture" behavior is much nicer than mc146818rtc's UIP, and ds1338 also
has no alarm and no interrupts), the bug should be almost trivial to
test for.

> And your suggestion that it is better to leave a known bug unpatched
> until someone can conjure up a test case is ridiculous. I don't see how
> that attitude help users, in the short or long term.

Of course some common sense is in order, as usual.  Leaving bugs
unpatched is bad, but prodding contributors and other maintainers is good.

Paolo
Andreas Färber - Feb. 15, 2013, 4:40 p.m.
Am 15.02.2013 12:41, schrieb Peter Maydell:
> On 15 February 2013 11:24, Andreas Färber <afaerber@suse.de> wrote:
>> Am 15.02.2013 03:49, schrieb Antoine Mathys:
>>> First, the ds1338 code was in a poor state and never handled the 12 hour
>>> clock correctly. My first patch failed to fully fix the problem so I had
>>> to write a second one, but at no point did Peter or I introduce a
>>> regression, quite the opposite.
>>
>> Read closely, I never claimed *you* introduced a regression. What I have
>> rather been observing is a seemingly not-ending stream of bugfix patches
> 
> One patch is hardly a never-ending stream!

I was referring to the previous six as well, where I first brought up
the topic of qtest (before going on to implement I2C libqos after seeing
another I2C'ish bugfix which did turn out to be half-broken).

Andreas

Patch

diff --git a/hw/ds1338.c b/hw/ds1338.c
index 1aefa3b..9e6b490 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -59,8 +59,8 @@  static void capture_current_time(DS1338State *s)
     s->nvram[1] = to_bcd(now.tm_min);
     if (s->nvram[2] & HOURS_12) {
         int tmp = now.tm_hour;
-        if (tmp == 0) {
-            tmp = 24;
+        if (tmp % 12 == 0) {
+            tmp += 12;
         }
         if (tmp <= 12) {
             s->nvram[2] = HOURS_12 | to_bcd(tmp);
@@ -145,8 +145,8 @@  static int ds1338_send(I2CSlave *i2c, uint8_t data)
                 if (data & HOURS_PM) {
                     tmp += 12;
                 }
-                if (tmp == 24) {
-                    tmp = 0;
+                if (tmp % 12 == 0) {
+                    tmp -= 12;
                 }
                 now.tm_hour = tmp;
             } else {