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

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

[nix.wie.weg]

Hello,

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

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

With kind regards,
Tino H. Seifert

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

[Johannes Schindelin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="us-ascii", Size: 2523 bytes --]

> 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

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

[nix.wie.weg]

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

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

[Lonnie Mendez]

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.

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

[andrzej zaborowski]

Hi,

On 27/04/06, nix.wie.weg@gmx.de <nix.wie.weg@gmx.de> 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 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
>
>
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
>


--
balrog 2oo6

Dear Outlook users: Please remove me from your address books
http://www.newsforge.com/article.pl?sid=03/08/21/143258

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

[Johannes Schindelin]

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 spend as much time to understand what is going on as you did?
Then what does your work try to achieve?

Let me drive the point home one more time (in case you missed it). Sending a
huge patch is wrong, for a plethora of reasons. Expecting others to swallow
it is preposterous. Instead of trying to hide it (which might work in a
closed source world, where you can hide behind a suit, but which fails
utterly in Open Source, where everybody can see what you did), admit the
mistake, work a little more, clean up your patch into a nice patch series,
and make everybody happy.

Hth,
Dscho


-- 
"Feel free" - 10 GB Mailbox, 100 FreeSMS/Monat ...
Jetzt GMX TopMail testen: http://www.gmx.net/de/go/topmail

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

[Lonnie Mendez]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

Johannes Schindelin wrote:

>>>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.
>>    
>>
>
>Also kindly look into line 1417 and 1419 of your patch. You will find the
>renaming o f product_name to prod_name.
>
   That was my doing.

manufacturer_name
product_name

    I have a habit of making related variables short and uniform in length.

[-- Attachment #2: qemu-usb-linux-vars.diff --]
[-- Type: text/plain, Size: 5038 bytes --]

--- qemu/usb-linux.c	2006-04-27 08:18:38.000000000 -0500
+++ qemu/usb-linux.c	2006-04-27 08:26:04.000000000 -0500
@@ -44,9 +44,9 @@
 
 typedef int USBScanFunc(void *opaque, int bus_num, int addr,
                         int vendor_id, int product_id, 
-                        const char *spec_version,
-                        const char *manf_string,
-                        const char *prod_string);
+                        const char *specification_version,
+                        const char *manufacturer_string,
+                        const char *product_string);
 static int usb_host_find_device(int *pbus_num, int *paddr, 
                                 const char *devname);
 int usb_host_handle_close(USBDevice *opaque);
@@ -282,9 +282,9 @@
     char buf[1024];
     int bus_num, addr, device_count, product_id, vendor_id;
     int ret;
-    char manf_name[512];
-    char prod_name[512];
-    char spec_version[6];
+    char manufacturer_name[512];
+    char product_name[512];
+    char specification_version[6];
     
     f = fopen(USBDEVFS_PATH "/devices", "r");
     if (!f) {
@@ -303,7 +303,7 @@
             if (device_count && (vendor_id || product_id)) {
                 /* New device.  Add the previously discovered device.  */
                 ret = func(opaque, bus_num, addr, vendor_id, product_id,
-                           spec_version, manf_name, prod_name);
+                           specification_version, manufacturer_name, product_name);
                 if (ret)
                     goto the_end;
             }
@@ -313,9 +313,9 @@
             if (get_tag_value(buf, sizeof(buf), line, "Dev#=", " ") < 0)
                 goto fail;
             addr = atoi(buf);
-            strcpy(spec_version, "01.10");
-            strcpy(manf_name, "unknown");
-            strcpy(prod_name, "unknown");
+            strcpy(specification_version, "01.10");
+            strcpy(manufacturer_name, "unknown");
+            strcpy(product_name, "unknown");
             device_count++;
             product_id = 0;
             vendor_id = 0;
@@ -323,7 +323,7 @@
             if (get_tag_value(&buf[1], sizeof(buf) - 1, line, "Ver=", " ") < 0)
                 goto fail;
             buf[0] = '0';
-            pstrcpy(spec_version, sizeof(spec_version), buf);
+            pstrcpy(specification_version, sizeof(specification_version), buf);
         } else if (line[0] == 'P' && line[1] == ':') {
             if (get_tag_value(buf, sizeof(buf), line, "Vendor=", " ") < 0)
                 goto fail;
@@ -336,18 +336,18 @@
                 if (get_tag_value(buf, sizeof(buf), line, "Product=", "") < 0) {
                     goto fail;
                 } else {
-                    pstrcpy(prod_name, sizeof(prod_name), buf);
+                    pstrcpy(product_name, sizeof(product_name), buf);
                 }
             } else {
-                pstrcpy(manf_name, sizeof(manf_name), buf);
+                pstrcpy(manufacturer_name, sizeof(manufacturer_name), buf);
             }
         }
     fail: ;
     }
     if (device_count && (vendor_id || product_id)) {
         /* Add the last device.  */
-        ret = func(opaque, bus_num, addr, vendor_id, product_id, spec_version,
-                   manf_name, prod_name);
+        ret = func(opaque, bus_num, addr, vendor_id, product_id,
+                   specification_version, manufacturer_name, product_name);
     }
  the_end:
     fclose(f);
@@ -363,9 +363,9 @@
 
 static int usb_host_find_device_scan(void *opaque, int bus_num, int addr,
                                      int vendor_id, int product_id,
-                                     const char *spec_version,
-                                     const char *manf_string,
-                                     const char *prod_string)
+                                     const char *specification_version,
+                                     const char *manufacturer_string,
+                                     const char *product_string)
 {
     FindDeviceState *s = opaque;
     if (vendor_id == s->vendor_id &&
@@ -408,14 +408,14 @@
 
 static int usb_host_info_device(void *opaque, int bus_num, int addr, 
                                 int vendor_id, int product_id,
-                                const char *spec_ver,
-                                const char *manf_string,
-                                const char *prod_string)
+                                const char *specification_version,
+                                const char *manufacturer_string,
+                                const char *product_string)
 {
     term_printf("  Device host:%03d:%03d, Manufacturer %s, Product %s\n",
-                bus_num, addr, manf_string, prod_string );
+                bus_num, addr, manufacturer_string, product_string );
     term_printf("     VendorID:ProductID %04xx%04x, USB-Standard: %s\n",
-                vendor_id, product_id, spec_ver );
+                vendor_id, product_id, specification_version );
     return 0;
 }
 

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

[nix.wie.weg]

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

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

[nix.wie.weg]

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

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

[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, nix.wie.weg@gmx.de 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
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFEUgHnmXZROF+EADURAtGkAJ43Iv/1wYTGK3RyNhiSPTiSet3T9wCbBC9H
+n+Tuu0lW1C8iwxs1bF20dc=
=s9uJ
-----END PGP SIGNATURE-----

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

[Per Åstrand]

[-- Attachment #1: Type: text/plain, Size: 5616 bytes --]

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
>
> > http://lists.nongnu.org/mailman/listinfo/qemu-devel
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.2.4 (GNU/Linux)
>
>
> iD8DBQFEUgHnmXZROF+EADURAtGkAJ43Iv/1wYTGK3RyNhiSPTiSet3T9wCbBC9H
> +n+Tuu0lW1C8iwxs1bF20dc=
> =s9uJ
> -----END PGP SIGNATURE-----
>
>
> _______________________________________________
> Qemu-devel mailing list
>
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
>

[-- Attachment #2: Type: text/html, Size: 7095 bytes --]