Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch
Hello! I just wanted to hear about the Large USB patch that was discussed quite alot here a couple of months ago. As I understood it the patch was put on hold until the 0.90 release, but I've heard nothing more about it... Is anybody (Tino, Lonnie?) working on getting the patch into CVS? Would be a shame if all the hard work was lost and someone had to start from scratch... /Per Sun, 20 Aug 2006 09:05:33 -0700 Joseph Miller -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 With all due respect, Tino Seifert, I must comment on your USB patch. I do not normally post to this list, but I follow it very closely as the project is of great interest to me. I must admit that I have rarely even looked at qemu code, and that I have never even looked at your USB patch. Personally, I can't wait for a USB patch to come out that will make USB life much easier. Qemu is a project that is about what the people want, and I think that the people want USB to work. However, the first people that you must work with are the other developers. If you wrote the greatest patch/bugfix that Qemu has ever seen, but you piss everyone off, no one will listen to you, and the many people that could have benefited from the patch will never see it's effects. I think that everyone on the list would love to see improvement to the USB functionality. We can all see that you have placed an immense amount of time and effort making this come to fruition. It sucks when so much time is spent on a project, especially one that works so well and has so much to offer, yet it must be delayed or modified to meet the expectations of the others. There are probably few people who know the Qemu USB code better than you do. Unfortunately, this becomes your problem. I think that most people want to see your patch integrated, but not all at once. I'm sure that it will require some serious re-working in order to satisfy the demands of the other developers. But policies are in place for a reason. Someone, somewhere, long ago in the history of coding, wrote lots of big patches every time they wanted to modify code, and they screwed it up beyond recognition. This may not be your fault, but you must pay the price for it. Suffice this to say, I would love to see your USB changes come about and I can't wait to test this once it is applied to CVS, but if changes in that patch don't come about, it will never happen. And somewhere down the line, probably months later, someone else will send in the same work that you did, in smaller chunks, they will work hard to try and cooperate with other demanding developers, and they will get all the credit for all of the work that you have already done. Please, I beg of you, make your patch worthwhile, make the requested changes, or I fear that we will never see your work. Sincerely, - -Joseph, A Discouraged Qemu User On Thursday 27 April 2006 11:45 am, [EMAIL PROTECTED] wrote: Hello Lonnie. Lonnie Mendez wrote: Johannes Schindelin wrote: Seeing as there is a release coming up this is most definitely not a good thing. Initial testing yielded lots of this. I'd like to see my all-in-one patch stripped out. Then simply modifying the linux redirector to support the improved error handling (have it clear endpoint halt/etc) and other improvements. Later, the new redirectors can be merged in and modified as necessary. Nobody said anything about a short to come release. I have no problem to suspend the patch until after this release. But then usb should stay as untouched as possible. The purpose of modifying the user interface to the usb layer also confuses me. What was the reasoning behind changing host: busaddr.addr to host:busaddr:addr and host:VID:PID to host:VIDxPID? This is something that should be abstracted in the layer and not handed down to the user. Why display the bcdUSB revision and not the connected speed to the user (as is already done)? The reason was a simple thougth of mine: a linux user could probably better understand bus:device (it is something which he gets displayed sometimes from libusb: libusb:001:002) but I m really open to change it to whatever string you want. A other possibility is that we introduce something like hostid:VID:PID. I can only tell you that I do not completely like the old notation. (But as I said before, no hard feelings about that, it is changed in less than a minute) The reason for the bcdUSB revision is even simpler. A person who has only used USB has no idea what full speed or lowspeed does mean. If you look through the code you will notice that the speed setting was determined by bcdUSB and so I thougth it may be better to display it. As most people have a idea what USB 1.0, 1.1 or 2.0 means. With kind regards, Tino H. Seifert ___ Qemu-devel mailing list Qemu-devel@nongnu.org
Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch
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) 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. Seeing as there is a release coming up this is most definitely not a good thing. Initial testing yielded lots of this. I'd like to see my all-in-one patch stripped out. Then simply modifying the linux redirector to support the improved error handling (have it clear endpoint halt/etc) and other improvements. Later, the new redirectors can be merged in and modified as necessary. The purpose of modifying the user interface to the usb layer also confuses me. What was the reasoning behind changing host:busaddr.addr to host:busaddr:addr and host:VID:PID to host:VIDxPID? This is something that should be abstracted in the layer and not handed down to the user. Why display the bcdUSB revision and not the connected speed to the user (as is already done)? To argue that this must all go in at once or none at all is silly. I've seen the changes and know that my redirectors aren't necessary for this to function. I'm not trying to get anyone down on this but am just saying this needs more discussion and thought. ___ Qemu-devel mailing list Qemu-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/qemu-devel
Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch
Hi, On 27/04/06, [EMAIL PROTECTED] [EMAIL PROTECTED] wrote: 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. Please check the www archive. 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. You could see Fabrice Bellard doing it now -- dividing the huge patch into small consistent commits, so it definitely is possible. And this is not the maintainer's task, it is normally done by the person who submits the changes before these changes can be considered for integrating (at least in theory). 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. Yes, that's one of the disadvantages of CVS which are very well dealt with in GIT (don't know about other systems). 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
Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch
Hi, nix.wie.weg wrote: 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. It was more polite. That is why you may have missed it. http://lists.gnu.org/archive/html/qemu-devel/2006-04/msg00456.html 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. You really try to mock me? I have not heard anyone complaining. They haven't had time to test it! If I find obvious errors on first sight of such a huge patch, it is no question there are more subtle bugs hidden away. Let me tell you a story from my day-job life. There was a man who deemed himself a programmer. One day he decided (without any budget or hint of the project leader to do so) to rip out a whole database query and reimplement that in pure C++ code. He argued that the database would be too slow to handle that. When we proved to him (I had to take out a pencil and a sheet of paper!) that all of a sudden, the application went from 32MB memory usage to 512MB. That and other points (the obvious being that Oracle's DBMS is *designed* to handle such a task much more gracefully) were such a big problem, even the suits got the point. What was his answer? You have to switch to Microsoft Terminal Services, then. What is the point of this story? If you make a mistake, admit it and correct it. Don't whine, and certainly do not suggest more work and money and hassles to others. It will only make you look like a fool. 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. Wrong. Now the maintainer has to do it. Which is you fault, not his. 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. grep your huge patch for usb_device_add and notice all the - in front, then repeat with usbtree_device_add, and notice all the +. Also kindly look into line 1417 and 1419 of your patch. You will find the renaming o f product_name to prod_name. 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. I read a lot of Fabrice's code. I am not the only one. Fabrice writes clear structured code which does not need many comments. I will not quote the rest of the paragraph which is just nasty towards Fabrice and the other authors. It's preposterous. 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. Apples and nuclear submarines? I track QEmu CVS with GIT, and have no such problems as you described. To the contrary, the commit history cannot contain holes, whereas everybody can forget a chagelog line (it only gets bad if somebody commits a *huge* patch with inadequate message). And BTW, I quoted the chagelog, because it has a typo. It also has a typo in the year, backdating your changes one year. It also has a semantic typo, in that the changes were not done on one single day, the 20th of April. There are so many errors of form that I *expect* the changes to contain errors, too. 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. You know what? That is one of the reasons, why small patches are Best Practice. You could not possibly have missed that if you had a series of small patches. Or if you read that patch even once. 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. :) If I did not know it better, I would think you tried to piss me off there. You want me to
Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch
Hello Johannes, I have to admit that your post did piss me off a little bit. But on the other hand it would not help anybody, if I would shoot back, so I will try to be as polite as possible. My first note is on your first message, which I really have not received. It is probably my fault, but I would have answered it, if I had got it. I did read it now in the archive - so sorry for that. If someone would seriously claim that well structured source code needs no comments, I would say he is an idiot (it is as stupid as the You have to switch to Microsoft Terminal Services, then. answer), but I'm *not* calling you an idiot. In fact I belive you were a little far-reaching in defending Fabrice's work. If you would lean back and lock from some distance to the current usb cvs source code (except usb-uhci.c) you will admit, that it is a quick and dirty work. This is not, as you suggested, nasty to Fabrice. Sometimes it is much better to implement something as a quick and dirty job than to not implement it at all. I do *really* appreciate Fabrice's work. You made some assumptions about large patches to linux. First of all you compared apples to oranges. A initial usb support patch as was introduced to qemu between version 0.7 and 0.8 would never have been accepted into the linux kernel. (- quick and dirty work) so on the other hand there would have been also no necessity for me to submit such an large patch. On the other hand there are or at least were these large patches to the linux kernel, when a new device or device class was implemented. In most cases these large patches are now first tested on some other kernel tree and discussed there. But for qemu there is no such other community. You have a great talent to pick examples. (the ones about usb_device_add ...) You will probably find some source lines where I have made such changes without any good reason, but the ones you have cited here are not of that category. The product_name change was introduced by Lonnie Mendez (read his post about that) and I think they are ok. It's nothing someone should be required to discuss long about, there is no reason not to rename them. The other example you have choosen is the one I have changed with my last patch which I had submitted with the documentation. The reason for this patch was simply that there were two usb_device_add functions (same name different functionality). Last but not least I have to tell you, that I do not like the way you are looking at open source development (at least what you have revealed in your last post). There is no way of telling people what they have to do or what they shouldn't. You can politly ask them, if they would change something, but you can't order them to do it and you should never ever try. If you ask someone politely to do things in one or the other way (like Fabrice did) you have to be alerted that someone will refuse to do so (as I did). I said no because I could not imagine how I should do it (there are some exceptions, read below). If someone else thinks he can do it. Then it is his task to stand up here and show that it could be done. I have a lot of doubt if it is possible to reach the same consistent, well documented and good structured source code. But I really would like to be proven wrong, we all would win on that. I'm aware that this applies also to me and my patch. I can not tell you to commit this patch I only can ask you (politely) about that. If you refuse to do so, it is absolutely ok. That was the point when I had said: Sometimes you have to take what you get. There may be things which can easily be extracted from the patch. One of these things Fabrice picked up already (usb-hub.c). The other files are (usb-libusb.c) and (usb-bsd.c) this saves around 1000 lines of the patch. But it does not do any harm to add them to a large patch, as I did. The reason for adding them was that I work with usb-libusb.c all day long and it would be great to have this file in the cvs tree. The file does do no harm and does not mean significant more work to the person who is applying the patch. There is also a other reason why I have included it, to make it easier for people outside the cvs applying process to test the patch with different os platforms. With kind regards, Tino H. Seifert ___ Qemu-devel mailing list Qemu-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/qemu-devel
Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch
Hello Lonnie. Lonnie Mendez wrote: Johannes Schindelin wrote: Seeing as there is a release coming up this is most definitely not a good thing. Initial testing yielded lots of this. I'd like to see my all-in-one patch stripped out. Then simply modifying the linux redirector to support the improved error handling (have it clear endpoint halt/etc) and other improvements. Later, the new redirectors can be merged in and modified as necessary. Nobody said anything about a short to come release. I have no problem to suspend the patch until after this release. But then usb should stay as untouched as possible. The purpose of modifying the user interface to the usb layer also confuses me. What was the reasoning behind changing host:busaddr.addr to host:busaddr:addr and host:VID:PID to host:VIDxPID? This is something that should be abstracted in the layer and not handed down to the user. Why display the bcdUSB revision and not the connected speed to the user (as is already done)? The reason was a simple thougth of mine: a linux user could probably better understand bus:device (it is something which he gets displayed sometimes from libusb: libusb:001:002) but I m really open to change it to whatever string you want. A other possibility is that we introduce something like hostid:VID:PID. I can only tell you that I do not completely like the old notation. (But as I said before, no hard feelings about that, it is changed in less than a minute) The reason for the bcdUSB revision is even simpler. A person who has only used USB has no idea what full speed or lowspeed does mean. If you look through the code you will notice that the speed setting was determined by bcdUSB and so I thougth it may be better to display it. As most people have a idea what USB 1.0, 1.1 or 2.0 means. With kind regards, Tino H. Seifert ___ Qemu-devel mailing list Qemu-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/qemu-devel
Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch
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 ___ Qemu-devel mailing list Qemu-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/qemu-devel
Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch
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
