| Message ID | 1355525607-2398-1-git-send-email-barsamin@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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
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
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.
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
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
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
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.
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
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
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 {
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(-)