Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch

[Johannes Schindelin]

> As Fabrice pointed out to me yesterday, it takes time to understand the
> new usb api. To make this process easier I have assembled a small
> documentation.
> You will find it here:
> http://217.20.126.200/tino/usb_api.pdf
> http://217.20.126.200/tino/usb_api.odg

That is a nice description.

> the patch which applies cleanly against todays cvs version is here,
> (which includes some small naming changes, as I discovered that I have
> used two times the same name for different functions. This was not a
> problem as this were local functions, but it was not so good for
> understanding the new api):
> http://217.20.126.200/tino/usb-2006-04-26.patch

I am quite sure you put a lot of work into this patch, but you sure make it
hard to appreciate, too.

First note that applying such a huge patch is bad. Let me help you (a little
more than last time) to understand that: You are almost guaranteed to
introduce bugs, and what's worse, regressions. Because it is so huge, you
are further guaranteed to have a real hard time tracking that regression.

Also, you make it really, really awkward to pick the changes which are
unquestionable, and skip the questionable ones.

For example, your patch "fixes" a few white spaces. This does not hurt, but
has no meaning either, and slows down patch reading.

Also, you rename some things when it has no apparent use to rename them,
such as usb_device_add, or product_name.

Parts of the patch are plainly unreadable, because you move code around. I
suppose you not only moved them around, but made subtle changes --
hard-to-notice-because-moved changes.

As of now, no files in CVS that I am aware of contain "chagelog" lines.

However, these reveal that you made changes as early as 20th April of 2005?
Plenty of time feeding small patches since then ;-)

Continuing with comments: you change the comment "QEMU USB HUB emulation" in
usb-hub.c to "QEMU PC System Emulator" and backdate Fabrice's copyright from
2005 to 2003-2004? Why?

In the patch, you made plenty of white-space changes which make it very
difficult to spot real changes.

Again, I think you did some very valuable work. However, I'd be so much
happier if you split the patch up into meaningful patches, not a rollup
which is hard to understand *and* huge.

I now spent about one hour trying to understand your fixes, and I know
almost as much as when I started about your fixes.

Hope this helps,
Dscho


-- 
Echte DSL-Flatrate dauerhaft für 0,- Euro*!
"Feel free" mit GMX DSL! http://www.gmx.net/de/go/dsl