[Qemu-devel] [PATCH 0/4] Minor USB fixes

[Qemu-devel] [PATCH 0/4] Minor USB fixes

[Brad Hards]

1 fixes spellos in the mass-storage driver
2-4 fix issues in Linux usb pass-through code. 

[Qemu-devel] [PATCH 1/4] usb: trivial spelling fixes

[Brad Hards]

Signed-off-by: Brad Hards <bradh@frogmouth.net>
---
 hw/usb-msd.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 76f5b02..947fd3f 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -33,7 +33,7 @@ do { printf("usb-msd: " fmt , ## __VA_ARGS__); } while (0)
 
 enum USBMSDMode {
     USB_MSDM_CBW, /* Command Block.  */
-    USB_MSDM_DATAOUT, /* Tranfer data to device.  */
+    USB_MSDM_DATAOUT, /* Transfer data to device.  */
     USB_MSDM_DATAIN, /* Transfer data from device.  */
     USB_MSDM_CSW /* Command Status.  */
 };
@@ -253,7 +253,7 @@ static void usb_msd_command_complete(SCSIBus *bus, int reason, uint32_t tag,
         usb_msd_copy_data(s);
         if (s->usb_len == 0) {
             /* Set s->packet to NULL before calling usb_packet_complete
-               because annother request may be issued before
+               because another request may be issued before
                usb_packet_complete returns.  */
             DPRINTF("Packet complete %p\n", p);
             s->packet = NULL;
-- 
1.7.1

[Qemu-devel] [PATCH 2/4] usb: initialise data element in Linux USB_DISCONNECT ioctl.

[Brad Hards]

This isn't used, but leaving it empty causes valgrind noise.

Signed-off-by: Brad Hards <bradh@frogmouth.net>
---
 usb-linux.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 255009f..d958853 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -344,6 +344,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
         for (interface = 0; interface < nb_interfaces; interface++) {
             ctrl.ioctl_code = USBDEVFS_DISCONNECT;
             ctrl.ifno = interface;
+            ctrl.data = 0;
             ret = ioctl(dev->fd, USBDEVFS_IOCTL, &ctrl);
             if (ret < 0 && errno != ENODATA) {
                 perror("USBDEVFS_DISCONNECT");
-- 
1.7.1

[Qemu-devel] [PATCH 3/4] usb: fix spelling errors in usb-linux.c

[Brad Hards]

Signed-off-by: Brad Hards <bradh@frogmouth.net>
---
 usb-linux.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index d958853..1f33c2c 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -107,7 +107,7 @@ enum {
 /*
  * Control transfer state.
  * Note that 'buffer' _must_ follow 'req' field because
- * we need contigious buffer when we submit control URB.
+ * we need contiguous buffer when we submit control URB.
  */
 struct ctrl_struct {
     uint16_t len;
@@ -580,7 +580,7 @@ static int usb_host_handle_control(USBHostDevice *s, USBPacket *p)
     /*
      * Setup ctrl transfer.
      *
-     * s->ctrl is layed out such that data buffer immediately follows
+     * s->ctrl is laid out such that data buffer immediately follows
      * 'req' struct which is exactly what usbdevfs expects.
      */
     urb = &aurb->urb;
-- 
1.7.1

[Qemu-devel] [PATCH 4/4] usb: use DPRINTF instead of printf for some simple cases

[Brad Hards]

Signed-off-by: Brad Hards <bradh@frogmouth.net>
---
 usb-linux.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 1f33c2c..b02a0f9 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -233,8 +233,8 @@ static void async_complete(void *opaque)
                 return;
             }
             if (errno == ENODEV && !s->closing) {
-                printf("husb: device %d.%d disconnected\n",
-                       s->bus_num, s->addr);
+                DPRINTF("husb: device %d.%d disconnected\n",
+                        s->bus_num, s->addr);
                 usb_host_close(s);
                 usb_host_auto_check(NULL);
                 return;
@@ -320,7 +320,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
         }
         config_descr_len = dev->descr[i];
 
-        printf("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
+        DPRINTF("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
 
         if (configuration < 0 || configuration == dev->descr[i + 5]) {
             configuration = dev->descr[i + 5];
@@ -359,7 +359,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
         ret = ioctl(dev->fd, USBDEVFS_CLAIMINTERFACE, &interface);
         if (ret < 0) {
             if (errno == EBUSY) {
-                printf("husb: update iface. device already grabbed\n");
+                DPRINTF("husb: update iface. device already grabbed\n");
             } else {
                 perror("husb: failed to claim interface");
             }
@@ -368,8 +368,8 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
         }
     }
 
-    printf("husb: %d interfaces claimed for configuration %d\n",
-           nb_interfaces, configuration);
+    DPRINTF("husb: %d interfaces claimed for configuration %d\n",
+            nb_interfaces, configuration);
 
     dev->ninterfaces   = nb_interfaces;
     dev->configuration = configuration;
@@ -929,7 +929,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
     if (dev->fd != -1) {
         goto fail;
     }
-    printf("husb: open device %d.%d\n", bus_num, addr);
+    DPRINTF("husb: open device %d.%d\n", bus_num, addr);
 
     if (!usb_host_device_path) {
         perror("husb: USB Host Device Path not set");
@@ -984,7 +984,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
         goto fail;
     }
 
-    printf("husb: grabbed usb device %d.%d\n", bus_num, addr);
+    DPRINTF("husb: grabbed usb device %d.%d\n", bus_num, addr);
 
     ret = usb_linux_update_endp_table(dev);
     if (ret) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 4/4] usb: use DPRINTF instead of printf for some simple cases

[Stefan Hajnoczi]

On Wed, Apr 13, 2011 at 10:45 AM, Brad Hards <bradh@frogmouth.net> wrote:
> Signed-off-by: Brad Hards <bradh@frogmouth.net>
> ---
>  usb-linux.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)

Some of these printfs look like they might be useful given that the
current USB support is known to be imperfect and frequently causes
questions from users.  By changing them to DPRINTF() you are making
these message available only to developers and not users.

Any thoughts from people who use or have written the usb-linux.c code?

Stefan

Re: [Qemu-devel] [PATCH 4/4] usb: use DPRINTF instead of printf for some simple cases

[Hans de Goede]

Hi,

On 04/13/2011 11:45 AM, Brad Hards wrote:
> Signed-off-by: Brad Hards<bradh@frogmouth.net>
> ---
>   usb-linux.c |   16 ++++++++--------
>   1 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/usb-linux.c b/usb-linux.c
> index 1f33c2c..b02a0f9 100644
> --- a/usb-linux.c
> +++ b/usb-linux.c
> @@ -233,8 +233,8 @@ static void async_complete(void *opaque)
>                   return;
>               }
>               if (errno == ENODEV&&  !s->closing) {
> -                printf("husb: device %d.%d disconnected\n",
> -                       s->bus_num, s->addr);
> +                DPRINTF("husb: device %d.%d disconnected\n",
> +                        s->bus_num, s->addr);
>                   usb_host_close(s);
>                   usb_host_auto_check(NULL);
>                   return;

I think this one should stay a regular printf, in case the disconnect is
unintentional people may think it is a qemu problem without the printf.

> @@ -320,7 +320,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
>           }
>           config_descr_len = dev->descr[i];
>
> -        printf("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
> +        DPRINTF("husb: config #%d need %d\n", dev->descr[i + 5], configuration);
>
>           if (configuration<  0 || configuration == dev->descr[i + 5]) {
>               configuration = dev->descr[i + 5];

Ack.

> @@ -359,7 +359,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
>           ret = ioctl(dev->fd, USBDEVFS_CLAIMINTERFACE,&interface);
>           if (ret<  0) {
>               if (errno == EBUSY) {
> -                printf("husb: update iface. device already grabbed\n");
> +                DPRINTF("husb: update iface. device already grabbed\n");
>               } else {
>                   perror("husb: failed to claim interface");
>               }

Nack, this is an error condition, so it should not be a DPRINTF.

> @@ -368,8 +368,8 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration)
>           }
>       }
>
> -    printf("husb: %d interfaces claimed for configuration %d\n",
> -           nb_interfaces, configuration);
> +    DPRINTF("husb: %d interfaces claimed for configuration %d\n",
> +            nb_interfaces, configuration);
>
>       dev->ninterfaces   = nb_interfaces;
>       dev->configuration = configuration;

Ack.

> @@ -929,7 +929,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
>       if (dev->fd != -1) {
>           goto fail;
>       }
> -    printf("husb: open device %d.%d\n", bus_num, addr);
> +    DPRINTF("husb: open device %d.%d\n", bus_num, addr);
>
>       if (!usb_host_device_path) {
>           perror("husb: USB Host Device Path not set");

Ack.

> @@ -984,7 +984,7 @@ static int usb_host_open(USBHostDevice *dev, int bus_num,
>           goto fail;
>       }
>
> -    printf("husb: grabbed usb device %d.%d\n", bus_num, addr);
> +    DPRINTF("husb: grabbed usb device %d.%d\n", bus_num, addr);
>
>       ret = usb_linux_update_endp_table(dev);
>       if (ret) {

Ack.

Re: [Qemu-devel] [PATCH 4/4] usb: use DPRINTF instead of printf for some simple cases

[Brad Hards]

On Wed, 13 Apr 2011 10:52:37 pm Hans de Goede wrote:
> > @@ -359,7 +359,7 @@ static int usb_host_claim_interfaces(USBHostDevice
> > *dev, int configuration)
> >
> >           ret = ioctl(dev->fd, USBDEVFS_CLAIMINTERFACE,&interface);
> >           if (ret<  0) {
> >               if (errno == EBUSY) {
> >
> > -                printf("husb: update iface. device already grabbed\n");
> > +                DPRINTF("husb: update iface. device already grabbed\n");
> >
> >               } else {
> >                   perror("husb: failed to claim interface");
> >               }
> 
> Nack, this is an error condition, so it should not be a DPRINTF.
Then should it go to stderr instead of stdout?

(There are other places in this code where we use fprintf(stderr, ...) to 
indicate error conditions.)

Brad

Re: [Qemu-devel] [PATCH 4/4] usb: use DPRINTF instead of printf for some simple cases

[Brad Hards]

On Thursday 14 April 2011 08:01:43 Brad Hards wrote:
> On Wed, 13 Apr 2011 10:52:37 pm Hans de Goede wrote:
> > > @@ -359,7 +359,7 @@ static int usb_host_claim_interfaces(USBHostDevice
> > > *dev, int configuration)
> > > 
> > >           ret = ioctl(dev->fd, USBDEVFS_CLAIMINTERFACE,&interface);
> > >           if (ret<  0) {
> > >           
> > >               if (errno == EBUSY) {
> > > 
> > > -                printf("husb: update iface. device already
> > > grabbed\n"); +                DPRINTF("husb: update iface. device
> > > already grabbed\n");
> > > 
> > >               } else {
> > >               
> > >                   perror("husb: failed to claim interface");
> > >               
> > >               }
> > 
> > Nack, this is an error condition, so it should not be a DPRINTF.
> 
> Then should it go to stderr instead of stdout?
> 
> (There are other places in this code where we use fprintf(stderr, ...) to
> indicate error conditions.)
ping?

Brad

Re: [Qemu-devel] [PATCH 0/4] Minor USB fixes

[Stefan Hajnoczi]

On Wed, Apr 13, 2011 at 10:45 AM, Brad Hards <bradh@frogmouth.net> wrote:
> 1 fixes spellos in the mass-storage driver
> 2-4 fix issues in Linux usb pass-through code.

I left a comment on patch 4 and will wait for activity on that email
thread.  Otherwise these could go via the trivial-patches tree and
I'll add them once discussion has finished.

Stefan

Re: [Qemu-devel] [PATCH 0/4] Minor USB fixes

[Stefan Hajnoczi]

On Wed, Apr 13, 2011 at 07:45:30PM +1000, Brad Hards wrote:
> 1 fixes spellos in the mass-storage driver
> 2-4 fix issues in Linux usb pass-through code. 

Patches 1-3 merged into the trivial patches tree.  Patch 4 should be a
separate patch and needs Ack from Hans.

I will send a pull request to the qemu.git maintainers:
http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches

For more information, see
http://wiki.qemu.org/Contribute/TrivialPatches.

Stefan