Patchwork [14/14] usb: Proper error propagation for usb_device_attach errors

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

Comments

Hans de Goede - May 31, 2011, 9:35 a.m.
---
 hw/usb-bus.c |   23 ++++++++++++-----------
 hw/usb-msd.c |    5 +++--
 usb-linux.c  |    6 +++++-
 3 files changed, 20 insertions(+), 14 deletions(-)
Michael Tokarev - May 31, 2011, 9:42 a.m.
31.05.2011 13:35, Hans de Goede wrote:
> ---
>  hw/usb-bus.c |   23 ++++++++++++-----------
>  hw/usb-msd.c |    5 +++--
>  usb-linux.c  |    6 +++++-
>  3 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
> index 0a49921..2ae2678 100644
> --- a/hw/usb-bus.c
> +++ b/hw/usb-bus.c

>      if (dev->attached) {
> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>                  dev->product_desc);

qemu_error() maybe, while we're at it?
Here and in a few other places.

Thanks!

/mjt
Hans de Goede - May 31, 2011, 9:51 a.m.
Hi,

On 05/31/2011 11:42 AM, Michael Tokarev wrote:
> 31.05.2011 13:35, Hans de Goede wrote:
>> ---
>>   hw/usb-bus.c |   23 ++++++++++++-----------
>>   hw/usb-msd.c |    5 +++--
>>   usb-linux.c  |    6 +++++-
>>   3 files changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>> index 0a49921..2ae2678 100644
>> --- a/hw/usb-bus.c
>> +++ b/hw/usb-bus.c
>
>>       if (dev->attached) {
>> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
>> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>>                   dev->product_desc);
>
> qemu_error() maybe, while we're at it?
> Here and in a few other places.

That does not seem to exist, do you perhaps mean error_printf() ?

Regards,

Hans
Kevin Wolf - May 31, 2011, 9:56 a.m.
Am 31.05.2011 11:51, schrieb Hans de Goede:
> Hi,
> 
> On 05/31/2011 11:42 AM, Michael Tokarev wrote:
>> 31.05.2011 13:35, Hans de Goede wrote:
>>> ---
>>>   hw/usb-bus.c |   23 ++++++++++++-----------
>>>   hw/usb-msd.c |    5 +++--
>>>   usb-linux.c  |    6 +++++-
>>>   3 files changed, 20 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>>> index 0a49921..2ae2678 100644
>>> --- a/hw/usb-bus.c
>>> +++ b/hw/usb-bus.c
>>
>>>       if (dev->attached) {
>>> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
>>> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>>>                   dev->product_desc);
>>
>> qemu_error() maybe, while we're at it?
>> Here and in a few other places.
> 
> That does not seem to exist, do you perhaps mean error_printf() ?

error_report() is what you should use, so that messages go to the
monitor if the function is called from a monitor command. error_printf()
is used by it internally, but usually isn't used directly.

Kevin
Hans de Goede - May 31, 2011, 10:05 a.m.
Hi,

On 05/31/2011 11:56 AM, Kevin Wolf wrote:
> Am 31.05.2011 11:51, schrieb Hans de Goede:
>> Hi,
>>
>> On 05/31/2011 11:42 AM, Michael Tokarev wrote:
>>> 31.05.2011 13:35, Hans de Goede wrote:
>>>> ---
>>>>    hw/usb-bus.c |   23 ++++++++++++-----------
>>>>    hw/usb-msd.c |    5 +++--
>>>>    usb-linux.c  |    6 +++++-
>>>>    3 files changed, 20 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>>>> index 0a49921..2ae2678 100644
>>>> --- a/hw/usb-bus.c
>>>> +++ b/hw/usb-bus.c
>>>
>>>>        if (dev->attached) {
>>>> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
>>>> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>>>>                    dev->product_desc);
>>>
>>> qemu_error() maybe, while we're at it?
>>> Here and in a few other places.
>>
>> That does not seem to exist, do you perhaps mean error_printf() ?
>
> error_report() is what you should use, so that messages go to the
> monitor if the function is called from a monitor command. error_printf()
> is used by it internally, but usually isn't used directly.
>

I've looked at error_report, but IMHO it is made of crazy, I'm not going
to construct a json dict every time I need to log some simple error message
(and the existing ones are not suitable for many error messages).

Regards,

Hans
Kevin Wolf - May 31, 2011, 10:12 a.m.
Am 31.05.2011 12:05, schrieb Hans de Goede:
> Hi,
> 
> On 05/31/2011 11:56 AM, Kevin Wolf wrote:
>> Am 31.05.2011 11:51, schrieb Hans de Goede:
>>> Hi,
>>>
>>> On 05/31/2011 11:42 AM, Michael Tokarev wrote:
>>>> 31.05.2011 13:35, Hans de Goede wrote:
>>>>> ---
>>>>>    hw/usb-bus.c |   23 ++++++++++++-----------
>>>>>    hw/usb-msd.c |    5 +++--
>>>>>    usb-linux.c  |    6 +++++-
>>>>>    3 files changed, 20 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>>>>> index 0a49921..2ae2678 100644
>>>>> --- a/hw/usb-bus.c
>>>>> +++ b/hw/usb-bus.c
>>>>
>>>>>        if (dev->attached) {
>>>>> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
>>>>> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>>>>>                    dev->product_desc);
>>>>
>>>> qemu_error() maybe, while we're at it?
>>>> Here and in a few other places.
>>>
>>> That does not seem to exist, do you perhaps mean error_printf() ?
>>
>> error_report() is what you should use, so that messages go to the
>> monitor if the function is called from a monitor command. error_printf()
>> is used by it internally, but usually isn't used directly.
>>
> 
> I've looked at error_report, but IMHO it is made of crazy, I'm not going
> to construct a json dict every time I need to log some simple error message
> (and the existing ones are not suitable for many error messages).

error_report() works with plain strings. Maybe you confuse it with the
QMP error reporting function?

Kevin
Hans de Goede - May 31, 2011, 10:13 a.m.
Hi,

On 05/31/2011 12:12 PM, Kevin Wolf wrote:
> Am 31.05.2011 12:05, schrieb Hans de Goede:
>> Hi,
>>
>> On 05/31/2011 11:56 AM, Kevin Wolf wrote:
>>> Am 31.05.2011 11:51, schrieb Hans de Goede:
>>>> Hi,
>>>>
>>>> On 05/31/2011 11:42 AM, Michael Tokarev wrote:
>>>>> 31.05.2011 13:35, Hans de Goede wrote:
>>>>>> ---
>>>>>>     hw/usb-bus.c |   23 ++++++++++++-----------
>>>>>>     hw/usb-msd.c |    5 +++--
>>>>>>     usb-linux.c  |    6 +++++-
>>>>>>     3 files changed, 20 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/usb-bus.c b/hw/usb-bus.c
>>>>>> index 0a49921..2ae2678 100644
>>>>>> --- a/hw/usb-bus.c
>>>>>> +++ b/hw/usb-bus.c
>>>>>
>>>>>>         if (dev->attached) {
>>>>>> -        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
>>>>>> +        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
>>>>>>                     dev->product_desc);
>>>>>
>>>>> qemu_error() maybe, while we're at it?
>>>>> Here and in a few other places.
>>>>
>>>> That does not seem to exist, do you perhaps mean error_printf() ?
>>>
>>> error_report() is what you should use, so that messages go to the
>>> monitor if the function is called from a monitor command. error_printf()
>>> is used by it internally, but usually isn't used directly.
>>>
>>
>> I've looked at error_report, but IMHO it is made of crazy, I'm not going
>> to construct a json dict every time I need to log some simple error message
>> (and the existing ones are not suitable for many error messages).
>
> error_report() works with plain strings. Maybe you confuse it with the
> QMP error reporting function?

Ah yes I was looking at qerror_report (who ever named that, having just
one letter difference in the function names is a bad idea). error_report
looks fine.

I'll wait a bit for more feedback and then change
[PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

To turn the fprintf(stderr, ... calls into error_report calls.

Thanks & Regards,

Hans



>
> Kevin
>
Gerd Hoffmann - June 1, 2011, 12:50 p.m.
Hi,

> I'll wait a bit for more feedback and then change
> [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors

I'll wait for new revisions of patches 12+14 then.

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

On 06/01/2011 02:50 PM, Gerd Hoffmann wrote:
> Hi,
>
>> I'll wait a bit for more feedback and then change
>> [PATCH 14/14] usb: Proper error propagation for usb_device_attach errors
>
> I'll wait for new revisions of patches 12+14 then.

I already fixed this yeterday, I opted to leave patch 12 unmodified and
replace all fprintf(stderr, ... calls with error_report calls in one go
in a new patch 14.

You can find the new version here:
http://cgit.freedesktop.org/~jwrdegoede/qemu/commit/?h=usb-patches&id=3c0be7730ff74a5cdac6aa100179b15c9392ab5f

Let me know if you'd rather have me resend 12 + 14 to the list.

Regards,

Hans
Gerd Hoffmann - June 6, 2011, 10:27 a.m.
Hi,

> Let me know if you'd rather have me resend 12 + 14 to the list.

I'd suggest to look at the leftover stuff after the next usb merge.

cheers,
   Gerd

PS: /me is busy doing tests, preparing the next usb pull which hopefully 
goes out later today.

Patch

diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 0a49921..2ae2678 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -75,7 +75,7 @@  static int usb_qdev_init(DeviceState *qdev, DeviceInfo *base)
     QLIST_INIT(&dev->strings);
     rc = dev->info->init(dev);
     if (rc == 0 && dev->auto_attach)
-        usb_device_attach(dev);
+        rc = usb_device_attach(dev);
     return rc;
 }
 
@@ -171,20 +171,20 @@  void usb_unregister_port(USBBus *bus, USBPort *port)
     bus->nfree--;
 }
 
-static void do_attach(USBDevice *dev)
+static int do_attach(USBDevice *dev)
 {
     USBBus *bus = usb_bus_from_device(dev);
     USBPort *port;
 
     if (dev->attached) {
-        fprintf(stderr, "Warning: tried to attach usb device %s twice\n",
+        fprintf(stderr, "Error: tried to attach usb device %s twice\n",
                 dev->product_desc);
-        return;
+        return -1;
     }
     if (bus->nfree == 0) {
-        fprintf(stderr, "Warning: tried to attach usb device %s to a bus with no free ports\n",
+        fprintf(stderr, "Error: tried to attach usb device %s to a bus with no free ports\n",
                 dev->product_desc);
-        return;
+        return -1;
     }
     if (dev->port_path) {
         QTAILQ_FOREACH(port, &bus->free, next) {
@@ -193,9 +193,9 @@  static void do_attach(USBDevice *dev)
             }
         }
         if (port == NULL) {
-            fprintf(stderr, "Warning: usb port %s (bus %s) not found\n",
+            fprintf(stderr, "Error: usb port %s (bus %s) not found\n",
                     dev->port_path, bus->qbus.name);
-            return;
+            return -1;
         }
     } else {
         port = QTAILQ_FIRST(&bus->free);
@@ -209,6 +209,8 @@  static void do_attach(USBDevice *dev)
 
     QTAILQ_INSERT_TAIL(&bus->used, port, next);
     bus->nused++;
+
+    return 0;
 }
 
 int usb_device_attach(USBDevice *dev)
@@ -220,8 +222,7 @@  int usb_device_attach(USBDevice *dev)
            (unless a physical port location is specified). */
         usb_create_simple(bus, "usb-hub");
     }
-    do_attach(dev);
-    return 0;
+    return do_attach(dev);
 }
 
 int usb_device_detach(USBDevice *dev)
@@ -230,7 +231,7 @@  int usb_device_detach(USBDevice *dev)
     USBPort *port;
 
     if (!dev->attached) {
-        fprintf(stderr, "Warning: tried to detach unattached usb device %s\n",
+        fprintf(stderr, "Error: tried to detach unattached usb device %s\n",
                 dev->product_desc);
         return -1;
     }
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index c8b7d41..68b46fa 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -481,8 +481,9 @@  static void usb_msd_password_cb(void *opaque, int err)
     MSDState *s = opaque;
 
     if (!err)
-        usb_device_attach(&s->dev);
-    else
+        err = usb_device_attach(&s->dev);
+
+    if (err)
         qdev_unplug(&s->dev.qdev);
 }
 
diff --git a/usb-linux.c b/usb-linux.c
index 490c49f..fe21da1 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1152,10 +1152,14 @@  static int usb_host_open(USBHostDevice *dev, int bus_num,
                 prod_name);
     }
 
+    ret = usb_device_attach(&dev->dev);
+    if (ret) {
+        goto fail;
+    }
+
     /* USB devio uses 'write' flag to check for async completions */
     qemu_set_fd_handler(dev->fd, NULL, async_complete, dev);
 
-    usb_device_attach(&dev->dev);
     return 0;
 
 fail: