Patchwork [07/14] usb-linux: If opening a device fails remove it from our filter list

login
register
mail settings
Submitter Hans de Goede
Date May 31, 2011, 9:35 a.m.
Message ID <1306834530-12763-8-git-send-email-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/97989/
State New
Headers show

Comments

Hans de Goede - May 31, 2011, 9:35 a.m.
So that we don't retry to open it every 2 seconds flooding stderr with
error messages.
---
 usb-linux.c |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)
Gerd Hoffmann - June 1, 2011, 12:32 p.m.
On 05/31/11 11:35, Hans de Goede wrote:
> So that we don't retry to open it every 2 seconds flooding stderr with
> error messages.

The polling here is done intentionally, so the devices catched by the 
filter show up in the guest automagically as soon as they are plugged 
in.  Just zapping the filter on failure isn't the right thing to do here.

We could try to do something more clever than polling sysfs every two 
seconds though, such using inotify to watch /sys/bus/usb/devices for new 
devices poping up.

cheers,
   Gerd
Hans de Goede - June 1, 2011, 2:37 p.m.
Hi,

On 06/01/2011 02:32 PM, Gerd Hoffmann wrote:
> On 05/31/11 11:35, Hans de Goede wrote:
>> So that we don't retry to open it every 2 seconds flooding stderr with
>> error messages.
>
> The polling here is done intentionally, so the devices catched by the filter show up in the guest automagically as soon as they are plugged in. Just zapping the filter on failure isn't the right thing to do here.

Note I'm zapping the filter when we fail to open the device, not when it
is not present. This can happen for example when the qemu user does not
have rights on the usbfs device node.

It seems better to me to print the relevant error once, and then require
the user to redo the usb_add / device_add if necessary, then to flood
the monitor with repeating the same error every 2 seconds. Note that
something like a permission problem (which is the most likely case
for opening a device failing once we've found it) won't go away by
itself.

> We could try to do something more clever than polling sysfs every two seconds though, such using inotify to watch /sys/bus/usb/devices for new devices poping up.

That would also result in trying to open the device ones, and if that
fails give up, I don't see the difference. Actually the user experience
would be worse, because the proper sequence in case of a permission
problem would go from:

usb_add
see error
fix permission
usb_add

to:

usb_add
see error
fix permission
usb_del
usb_add

Regards,

Hans
Gerd Hoffmann - June 6, 2011, 10:24 a.m.
On 06/01/11 16:37, Hans de Goede wrote:
> Hi,
>
> On 06/01/2011 02:32 PM, Gerd Hoffmann wrote:
>> On 05/31/11 11:35, Hans de Goede wrote:
>>> So that we don't retry to open it every 2 seconds flooding stderr with
>>> error messages.
>>
>> The polling here is done intentionally, so the devices catched by the
>> filter show up in the guest automagically as soon as they are plugged
>> in. Just zapping the filter on failure isn't the right thing to do here.
>
> Note I'm zapping the filter when we fail to open the device, not when it
> is not present. This can happen for example when the qemu user does not
> have rights on the usbfs device node.
>
> It seems better to me to print the relevant error once, and then require
> the user to redo the usb_add / device_add if necessary, then to flood
> the monitor with repeating the same error every 2 seconds.

Devices magically disappearing is a problem for the management stack 
(such as libvirt) though.  qemu does stuff like that in too many places 
already and we are trying to get rid of it.

Flooding the monitor (or stderr) indeed isn't nice though.

cheers,
   Gerd

Patch

diff --git a/usb-linux.c b/usb-linux.c
index e2f45d3..334012e 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -122,6 +122,7 @@  typedef struct USBHostDevice {
     int       configuration;
     int       ninterfaces;
     int       closing;
+    int       open_failed;
     Notifier  exit;
 
     struct endp_data endp_table[MAX_ENDPOINTS];
@@ -1208,6 +1209,10 @@  static int usb_host_initfn(USBDevice *dev)
     s->exit.notify = usb_host_exit_notifier;
     qemu_add_exit_notifier(&s->exit);
     usb_host_auto_check(NULL);
+    if (s->open_failed) {
+        usb_host_handle_destroy(dev);
+        return s->open_failed;
+    }
     return 0;
 }
 
@@ -1272,7 +1277,11 @@  USBDevice *usb_host_device_open(const char *devname)
     qdev_prop_set_uint32(&dev->qdev, "hostaddr",  filter.addr);
     qdev_prop_set_uint32(&dev->qdev, "vendorid",  filter.vendor_id);
     qdev_prop_set_uint32(&dev->qdev, "productid", filter.product_id);
-    qdev_init_nofail(&dev->qdev);
+
+    if (qdev_init(&dev->qdev) != 0) {
+        return NULL;
+    }
+
     return dev;
 
 fail:
@@ -1637,6 +1646,7 @@  static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
 {
     struct USBAutoFilter *f;
     struct USBHostDevice *s;
+    int ret;
 
     /* Ignore hubs */
     if (class_id == 9)
@@ -1670,7 +1680,15 @@  static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
         }
         DPRINTF("husb: auto open: bus_num %d addr %d\n", bus_num, addr);
 
-        usb_host_open(s, bus_num, addr, port, product_name, speed);
+        ret = usb_host_open(s, bus_num, addr, port, product_name, speed);
+        if (ret) {
+            /* If we've found a match but failed to open the device we should
+               remove it from our auto-filter list, so we don't keep trying to
+               open it every 2 seconds.  However we cannot destroy / free it
+               here, since we get called from usb_host_initfn, and destroying
+               a qdev from its initfn is not allowed. */
+            s->open_failed = ret;
+        }
     }
 
     return 0;
@@ -1678,9 +1696,16 @@  static int usb_host_auto_scan(void *opaque, int bus_num, int addr, char *port,
 
 static void usb_host_auto_check(void *unused)
 {
-    struct USBHostDevice *s;
+    struct USBHostDevice *s, *n;
     int unconnected = 0;
 
+    /* Cleanup devices wich failed to open last time, see usb_host_auto_scan
+       for why we don't do this after usb_host_auto_scan */
+    QTAILQ_FOREACH_SAFE(s, &hostdevs, next, n) {
+        if (s->open_failed)
+            qdev_free(&s->dev.qdev);
+    }
+
     usb_host_scan(NULL, usb_host_auto_scan);
 
     QTAILQ_FOREACH(s, &hostdevs, next) {