| Message ID | 1306834530-12763-15-git-send-email-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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 >
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
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
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.
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: