qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: nix . wie . weg
Subject: Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch
Date: Thu, 27 Apr 2006 03:23:55 +0200
User-agent: Mail/News 1.5 (X11/20060228)

Hello Johannes,

Thanks for your comments.

Johannes Schindelin wrote:
> 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) 
Sorry I dont know why, but I have no other message from you in my Inbox,
so I can't tell what you have written 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.
>   
I agree with you on that. But I have a reason for doing it and I have no
one heard complaining, this or that has been broken, which worked
before. On the other side, I can say for sure, that with that patch a
lot more things do work. So even if something may be broken and cvs
changes will with great probability break things (see softfloat-native.h
today), it does not mean the patch is bad. The only question which has
to be asked is, could it be done with smaller patches. And on that point
I disagree with you. I claim that it could not be done.
> 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.
>   
Sorry but on that point you say things which are in no case provable by
any study. The contrary is true, good readable code, many comments, good
structered function names, which follow a pattern, help to write good
and error free code. When I got my hands on the usb code, there were as
many as almost none comments. I needed more than one hour to even get a
glue what is going on in usb.h (which is apparently not the reason for
having header files). The usb code as I obtained it, was in a state
which you can describe in the words "quick and dirty". If you find such
conditions you have to change without mercy. If you don't do it and the
person, which has applied the first changes to cvs, has done a quick and
dirty job, you will always have a patch work software, without any
structure and without any consistent interface.  So maybe that the
person, who wants to commit the patch to cvs, has more work to do. But
this persons work is absolutly nonrelevant in compare to the many people
who will read the (then better) code, and understand it faster and
deeper. I'm absolutly sure, that you will find examples where I have not
choosen the best solution, but if I would look into any code which you
have written the same would be valid and that is not a problem as long
as the changes which are good outweigh the bad ones.
> 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.
Until the 19th century it was not common that a doctor has washed his
hands when he had evaluated a patient. This doesn't mean it is a bad
idea to do so. My experiences with other projects have shown, that it is
in most cases a good idea to have these changelog lines even if you use
cvs. But if the majority of the people here is against them, I will take
them out - that is not a problem. It is sometimes not so easy to find
the person who had developed a portion of source code after lets say 5
years. In the cvs you will normaly find only the person who has applied
the patch and not the person who has developed the code. If you then
want to contact the original developer it can be a hard job to find him.
So thats the reason for making these short changelog lines. Some
opensource licenses (not the one for qemu) even require such changelog
lines and I personaly believe, it is good to have them.
> 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?
>   
Yes sorry about that, it was not my intention to do so. Let me explain
how that was happening. As you may have noticed, Fabrice has added
usb-hub.c earlier this week, after I had already submitted my patch. As
I had updated my local cvs version this changes were rejected. And for
one or the other reason I had not noticed, the difference in the
comment. I haved changed it now.
> 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.
>
>   
One hour is nothing I had to spend one day to understand even the data
path between the different usb functions. :)
> Hope this helps,
> Dscho
>   
I too hope this helps,
Tino H. Seifert




reply via email to

[Prev in Thread] Current Thread [Next in Thread]