Patchwork [4/4] usb: use DPRINTF instead of printf for some simple cases

login
register
mail settings
Submitter Brad Hards
Date April 13, 2011, 9:45 a.m.
Message ID <1302687934-1287-5-git-send-email-bradh@frogmouth.net>
Download mbox | patch
Permalink /patch/90978/
State New
Headers show

Comments

Brad Hards - April 13, 2011, 9:45 a.m.
Signed-off-by: Brad Hards <bradh@frogmouth.net>
---
 usb-linux.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
Stefan Hajnoczi - April 13, 2011, 12:36 p.m.
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
Hans de Goede - April 13, 2011, 12:52 p.m.
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.
Brad Hards - April 13, 2011, 10:01 p.m.
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
Brad Hards - April 25, 2011, 1:02 a.m.
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

Patch

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) {