[Qemu-devel] [PATCH 0/7] usb: Improvements around device realization

[Qemu-devel] [PATCH 0/7] usb: Improvements around device realization

[Markus Armbruster]

Markus Armbruster (7):
  usb: usb_create() can't fail, drop useless error handling
  usb: Improve -usbdevice error reporting a bit
  usb: Do not prefix error_setg() messages with "Error: "
  usb: Suppress bogus error when automatic usb-hub creation fails
  usb: Change usb_create_simple() to abort on failure
  r2d: Don't use legacy -usbdevice support for setting up board
  PPC: Don't use legacy -usbdevice support for setting up board

 hw/ppc/mac_newworld.c  |  7 ++++--
 hw/ppc/spapr.c         |  7 ++++--
 hw/sh4/r2d.c           |  2 +-
 hw/usb/bus.c           | 64 +++++++++++++++++++++++++++++++++++---------------
 hw/usb/dev-bluetooth.c | 11 +--------
 hw/usb/dev-network.c   |  4 ----
 hw/usb/dev-serial.c    |  7 ------
 hw/usb/dev-storage.c   |  6 -----
 hw/usb/host-legacy.c   |  1 -
 9 files changed, 57 insertions(+), 52 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/7] usb: usb_create() can't fail, drop useless error handling

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/bus.c           | 4 ----
 hw/usb/dev-bluetooth.c | 6 +-----
 hw/usb/dev-network.c   | 3 ---
 hw/usb/dev-serial.c    | 3 ---
 hw/usb/dev-storage.c   | 3 ---
 5 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 986b2d8..eeb6872 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -320,10 +320,6 @@ USBDevice *usb_create_simple(USBBus *bus, const char *name)
     USBDevice *dev = usb_create(bus, name);
     int rc;
 
-    if (!dev) {
-        error_report("Failed to create USB device '%s'", name);
-        return NULL;
-    }
     rc = qdev_init(&dev->qdev);
     if (rc < 0) {
         error_report("Failed to initialize USB device '%s'", name);
diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c
index 390d475..8b3b316 100644
--- a/hw/usb/dev-bluetooth.c
+++ b/hw/usb/dev-bluetooth.c
@@ -530,14 +530,10 @@ static USBDevice *usb_bt_init(USBBus *bus, const char *cmdline)
     } else {
         hci = bt_new_hci(qemu_find_bt_vlan(0));
     }
-
     if (!hci)
         return NULL;
+
     dev = usb_create(bus, name);
-    if (!dev) {
-        error_report("Failed to create USB device '%s'", name);
-        return NULL;
-    }
     s = DO_UPCAST(struct USBBtState, dev, dev);
     s->hci = hci;
     if (qdev_init(&dev->qdev) < 0) {
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 5b95d5c..620fc11 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1405,9 +1405,6 @@ static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
     }
 
     dev = usb_create(bus, "usb-net");
-    if (!dev) {
-        return NULL;
-    }
     qdev_set_nic_properties(&dev->qdev, &nd_table[idx]);
     qdev_init_nofail(&dev->qdev);
     return dev;
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 1cee450..f347c05 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -544,9 +544,6 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
         return NULL;
 
     dev = usb_create(bus, "usb-serial");
-    if (!dev) {
-        return NULL;
-    }
     qdev_prop_set_chr(&dev->qdev, "chardev", cdrv);
     if (vendorid)
         qdev_prop_set_uint16(&dev->qdev, "vendorid", vendorid);
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 4539733..5f22275 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -706,9 +706,6 @@ static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
 
     /* create guest device */
     dev = usb_create(bus, "usb-storage");
-    if (!dev) {
-        return NULL;
-    }
     if (qdev_prop_set_drive(&dev->qdev, "drive",
                             blk_by_legacy_dinfo(dinfo)) < 0) {
         object_unparent(OBJECT(dev));
-- 
1.9.3

[Qemu-devel] [PATCH 2/7] usb: Improve -usbdevice error reporting a bit

[Markus Armbruster]

Most LegacyUSBFactory usbdevice_init() methods realize with
qdev_init_nofail(), even though their caller usbdevice_create() can
handle failure.  Okay if it really can't fail (I didn't check), but
somewhat brittle.

usb_msd_init() and usb_bt_init() call qdev_init().  The latter
additionally reports an error when qdev_init() fails.

Realization failure produces multiple error reports: a specific one
from qdev_init(), and generic ones from usb_bt_init(),
usb_create_simple(), usbdevice_create() and usb_parse().

Remove realization from the usbdevice_init() methods.  Realize in
usbdevice_create(), and produce exactly one error message there.  You
still get another one from usb_parse().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/bus.c           | 22 +++++++++++++++++++---
 hw/usb/dev-bluetooth.c |  5 -----
 hw/usb/dev-network.c   |  1 -
 hw/usb/dev-serial.c    |  4 ----
 hw/usb/dev-storage.c   |  3 ---
 hw/usb/host-legacy.c   |  1 -
 6 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index eeb6872..3f69fe1 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -651,10 +651,12 @@ USBDevice *usbdevice_create(const char *cmdline)
 {
     USBBus *bus = usb_bus_find(-1 /* any */);
     LegacyUSBFactory *f = NULL;
+    Error *err = NULL;
     GSList *i;
     char driver[32];
     const char *params;
     int len;
+    USBDevice *dev;
 
     params = strchr(cmdline,':');
     if (params) {
@@ -689,14 +691,28 @@ USBDevice *usbdevice_create(const char *cmdline)
         return NULL;
     }
 
-    if (!f->usbdevice_init) {
+    if (f->usbdevice_init) {
+        dev = f->usbdevice_init(bus, params);
+    } else {
         if (*params) {
             error_report("usbdevice %s accepts no params", driver);
             return NULL;
         }
-        return usb_create_simple(bus, f->name);
+        dev = usb_create(bus, f->name);
     }
-    return f->usbdevice_init(bus, params);
+    if (!dev) {
+        error_report("Failed to create USB device '%s'", f->name);
+        return NULL;
+    }
+    object_property_set_bool(OBJECT(dev), true, "realized", &err);
+    if (err) {
+        error_report("Failed to initialize USB device '%s': %s",
+                     f->name, error_get_pretty(err));
+        error_free(err);
+        object_unparent(OBJECT(dev));
+        return NULL;
+    }
+    return dev;
 }
 
 static void usb_device_class_init(ObjectClass *klass, void *data)
diff --git a/hw/usb/dev-bluetooth.c b/hw/usb/dev-bluetooth.c
index 8b3b316..9bf6730 100644
--- a/hw/usb/dev-bluetooth.c
+++ b/hw/usb/dev-bluetooth.c
@@ -536,11 +536,6 @@ static USBDevice *usb_bt_init(USBBus *bus, const char *cmdline)
     dev = usb_create(bus, name);
     s = DO_UPCAST(struct USBBtState, dev, dev);
     s->hci = hci;
-    if (qdev_init(&dev->qdev) < 0) {
-        error_report("Failed to initialize USB device '%s'", name);
-        return NULL;
-    }
-
     return dev;
 }
 
diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 620fc11..7131abd 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1406,7 +1406,6 @@ static USBDevice *usb_net_init(USBBus *bus, const char *cmdline)
 
     dev = usb_create(bus, "usb-net");
     qdev_set_nic_properties(&dev->qdev, &nd_table[idx]);
-    qdev_init_nofail(&dev->qdev);
     return dev;
 }
 
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index f347c05..67c2072 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -549,8 +549,6 @@ static USBDevice *usb_serial_init(USBBus *bus, const char *filename)
         qdev_prop_set_uint16(&dev->qdev, "vendorid", vendorid);
     if (productid)
         qdev_prop_set_uint16(&dev->qdev, "productid", productid);
-    qdev_init_nofail(&dev->qdev);
-
     return dev;
 }
 
@@ -565,8 +563,6 @@ static USBDevice *usb_braille_init(USBBus *bus, const char *unused)
 
     dev = usb_create(bus, "usb-braille");
     qdev_prop_set_chr(&dev->qdev, "chardev", cdrv);
-    qdev_init_nofail(&dev->qdev);
-
     return dev;
 }
 
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 5f22275..af2e1b9 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -711,9 +711,6 @@ static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
         object_unparent(OBJECT(dev));
         return NULL;
     }
-    if (qdev_init(&dev->qdev) < 0)
-        return NULL;
-
     return dev;
 }
 
diff --git a/hw/usb/host-legacy.c b/hw/usb/host-legacy.c
index 3cc9c42..422ed9a 100644
--- a/hw/usb/host-legacy.c
+++ b/hw/usb/host-legacy.c
@@ -128,7 +128,6 @@ USBDevice *usb_host_device_open(USBBus *bus, 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);
     return dev;
 
 fail:
-- 
1.9.3

[Qemu-devel] [PATCH 3/7] usb: Do not prefix error_setg() messages with "Error: "

[Markus Armbruster]

Because it produces beauties like

    (qemu) usb_add mouse
    Failed to initialize USB device 'usb-mouse': Error: tried to attach usb device QEMU USB Mouse to a bus with no free ports

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 3f69fe1..3e85afe 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -412,7 +412,7 @@ void usb_claim_port(USBDevice *dev, Error **errp)
             }
         }
         if (port == NULL) {
-            error_setg(errp, "Error: usb port %s (bus %s) not found (in use?)",
+            error_setg(errp, "usb port %s (bus %s) not found (in use?)",
                        dev->port_path, bus->qbus.name);
             return;
         }
@@ -422,7 +422,7 @@ void usb_claim_port(USBDevice *dev, Error **errp)
             usb_create_simple(bus, "usb-hub");
         }
         if (bus->nfree == 0) {
-            error_setg(errp, "Error: tried to attach usb device %s to a bus "
+            error_setg(errp, "tried to attach usb device %s to a bus "
                        "with no free ports", dev->product_desc);
             return;
         }
-- 
1.9.3

[Qemu-devel] [PATCH 4/7] usb: Suppress bogus error when automatic usb-hub creation fails

[Markus Armbruster]

USBDevice's realize method usb_qdev_realize() automatically creates a
usb-hub when only one port is left.  Creating devices in realize
methods is questionable, but works.

If usb-hub creation fails, an error is reported to stderr, but the
failure is otherwise ignored.  We then create the actual device using
the last port, which may well succeed.

Example:

    $ qemu -nodefaults -S -display none -machine usb=on -monitor stdio
    QEMU 2.2.50 monitor - type 'help' for more information
    (qemu) device_add usb-mouse
    [Repeat 36 times]
    (qemu) info usb
      Device 0.0, Port 1, Speed 12 Mb/s, Product QEMU USB Mouse
      Device 0.0, Port 2, Speed 12 Mb/s, Product QEMU USB Hub
      Device 0.0, Port 2.1, Speed 12 Mb/s, Product QEMU USB Mouse
    [More mice and hubs omitted...]
      Device 0.0, Port 2.8.8.8.8.7, Speed 12 Mb/s, Product QEMU USB Mouse
    (qemu) device_add usb-mouse
    usb hub chain too deep
    Failed to initialize USB device 'usb-hub'
    (qemu) info usb
    [...]
      Device 0.0, Port 2.8.8.8.8.7, Speed 12 Mb/s, Product QEMU USB Mouse
      Device 0.0, Port 2.8.8.8.8.8, Speed 12 Mb/s, Product QEMU USB Mouse

Despite the "Failed" message, the command actually succeeded.

In QMP, it's worse.  When adding the 37th mouse via QMP, the command
fails with

    {"error": {"class": "GenericError", "desc": "usb hub chain too deep"}}

Additionally, "Failed to initialize USB device 'usb-hub'" is reported
on stderr.  Despite the command failure, the device was created.  This
is wrong.

Fix by avoiding qdev_init() for usb-hub creation, so we can ignore
errors cleanly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/bus.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 3e85afe..5abfac0 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -315,15 +315,36 @@ USBDevice *usb_create(USBBus *bus, const char *name)
     return USB_DEVICE(dev);
 }
 
+static USBDevice *usb_try_create_simple(USBBus *bus, const char *name,
+                                        Error **errp)
+{
+    Error *err = NULL;
+    USBDevice *dev;
+
+    dev = USB_DEVICE(qdev_try_create(&bus->qbus, name));
+    if (!dev) {
+        error_setg(errp, "Failed to create USB device '%s'", name);
+        return NULL;
+    }
+    object_property_set_bool(OBJECT(dev), true, "realized", &err);
+    if (err) {
+        error_setg(errp, "Failed to initialize USB device '%s': %s",
+                   name, error_get_pretty(err));
+        error_free(err);
+        object_unparent(OBJECT(dev));
+        return NULL;
+    }
+    return dev;
+}
+
 USBDevice *usb_create_simple(USBBus *bus, const char *name)
 {
-    USBDevice *dev = usb_create(bus, name);
-    int rc;
+    Error *err = NULL;
+    USBDevice *dev = usb_try_create_simple(bus, name, &err);
 
-    rc = qdev_init(&dev->qdev);
-    if (rc < 0) {
-        error_report("Failed to initialize USB device '%s'", name);
-        return NULL;
+    if (!dev) {
+        error_report("%s", error_get_pretty(err));
+        error_free(err);
     }
     return dev;
 }
@@ -419,7 +440,7 @@ void usb_claim_port(USBDevice *dev, Error **errp)
     } else {
         if (bus->nfree == 1 && strcmp(object_get_typename(OBJECT(dev)), "usb-hub") != 0) {
             /* Create a new hub and chain it on */
-            usb_create_simple(bus, "usb-hub");
+            usb_try_create_simple(bus, "usb-hub", NULL);
         }
         if (bus->nfree == 0) {
             error_setg(errp, "tried to attach usb device %s to a bus "
-- 
1.9.3

[Qemu-devel] [PATCH 5/7] usb: Change usb_create_simple() to abort on failure

[Markus Armbruster]

Instead of returning null pointer.  Matches pci_create_simple(),
isa_create_simple(), sysbus_create_simple().  It's unused since the
previous commit, but I'll put it to use again shortly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/bus.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 5abfac0..d83a938 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -339,14 +339,7 @@ static USBDevice *usb_try_create_simple(USBBus *bus, const char *name,
 
 USBDevice *usb_create_simple(USBBus *bus, const char *name)
 {
-    Error *err = NULL;
-    USBDevice *dev = usb_try_create_simple(bus, name, &err);
-
-    if (!dev) {
-        error_report("%s", error_get_pretty(err));
-        error_free(err);
-    }
-    return dev;
+    return usb_try_create_simple(bus, name, &error_abort);
 }
 
 static void usb_fill_port(USBPort *port, void *opaque, int index,
-- 
1.9.3

[Qemu-devel] [PATCH 6/7] r2d: Don't use legacy -usbdevice support for setting up board

[Markus Armbruster]

It's tempting, because usbdevice_create() is so simple to use.  But
there's a lot of unwanted complexity behind the simple interface.
Switch to usb_create_simple().

Cc: Magnus Damm <magnus.damm@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/sh4/r2d.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 12f44d2..d1d0847 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -301,7 +301,7 @@ static void r2d_init(MachineState *machine)
                             "rtl8139", i==0 ? "2" : NULL);
 
     /* USB keyboard */
-    usbdevice_create("keyboard");
+    usb_create_simple(usb_bus_find(-1), "usb-kbd");
 
     /* Todo: register on board registers */
     memset(&boot_params, 0, sizeof(boot_params));
-- 
1.9.3

[Qemu-devel] [PATCH 7/7] PPC: Don't use legacy -usbdevice support for setting up board

[Markus Armbruster]

It's tempting, because usbdevice_create() is so simple to use.  But
there's a lot of unwanted complexity behind the simple interface.
Switch to usb_create_simple().

Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/mac_newworld.c | 7 +++++--
 hw/ppc/spapr.c        | 7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index c377012..624b4ab 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -420,11 +420,14 @@ static void ppc_core99_init(MachineState *machine)
 
     if (machine->usb) {
         pci_create_simple(pci_bus, -1, "pci-ohci");
+
         /* U3 needs to use USB for input because Linux doesn't support via-cuda
         on PPC64 */
         if (machine_arch == ARCH_MAC99_U3) {
-            usbdevice_create("keyboard");
-            usbdevice_create("mouse");
+            USBBus *usb_bus = usb_bus_find(-1);
+
+            usb_create_simple(usb_bus, "usb-kbd");
+            usb_create_simple(usb_bus, "usb-mouse");
         }
     }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b560459..5112373 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1533,9 +1533,12 @@ static void ppc_spapr_init(MachineState *machine)
 
     if (machine->usb) {
         pci_create_simple(phb->bus, -1, "pci-ohci");
+
         if (spapr->has_graphics) {
-            usbdevice_create("keyboard");
-            usbdevice_create("mouse");
+            USBBus *usb_bus = usb_bus_find(-1);
+
+            usb_create_simple(usb_bus, "usb-kbd");
+            usb_create_simple(usb_bus, "usb-mouse");
         }
     }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 7/7] PPC: Don't use legacy -usbdevice support for setting up board

[Alexander Graf]

On 04.02.15 13:28, Markus Armbruster wrote:
> It's tempting, because usbdevice_create() is so simple to use.  But
> there's a lot of unwanted complexity behind the simple interface.
> Switch to usb_create_simple().
> 
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex