Re: [Qemu-devel] [PATCH] Fix conversion between 12 hours and 24 hours modes.

[Andreas Färber]

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