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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
