[Qemu-devel] [PATCH 0/4] scsi: Error reporting cleanups, realize()

[Qemu-devel] [PATCH 0/4] scsi: Error reporting cleanups, realize()

[Markus Armbruster]

This series is based on
* [PULL 00/96] pci, pc, virtio fixes and cleanups
* [PULL 0/8] usb: error handling fixes from Markus, make sysbus ehci
  arm-only.
* [PULL v2 00/11] Clean up around error_get_pretty(),
  qerror_report_err()
* [PATCH 0/2] pci: Bury dead legacy commands pci_add, pci_del
* [PATCH] scsi: give device a parent before setting properties

Series passes make check with my "[PATCH RFC 1/1] qtest: Add generic
PCI device test" applied.

Markus Armbruster (4):
  scsi: Clean up duplicated error in legacy if=scsi code
  hw: Propagate errors through qdev_prop_set_drive()
  scsi: Improve error reporting for invalid drive property
  scsi: Convert remaining PCI HBAs to realize()

 hw/arm/vexpress.c                |  6 +++---
 hw/arm/virt.c                    |  6 +++---
 hw/block/pflash_cfi01.c          |  4 ++--
 hw/block/pflash_cfi02.c          |  4 ++--
 hw/core/qdev-properties-system.c | 22 ++++++++++------------
 hw/scsi/esp-pci.c                | 28 +++++++++++-----------------
 hw/scsi/lsi53c895a.c             | 13 +++----------
 hw/scsi/megasas.c                | 12 +++---------
 hw/scsi/scsi-bus.c               |  6 +++---
 hw/scsi/spapr_vscsi.c            |  2 ++
 hw/usb/dev-storage.c             |  7 +++++--
 include/hw/qdev-properties.h     |  4 ++--
 12 files changed, 49 insertions(+), 65 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/4] scsi: Clean up duplicated error in legacy if=scsi code

[Markus Armbruster]

Commit a818a4b changed scsi_bus_legacy_handle_cmdline() to report
errors from scsi_bus_legacy_add_drive() with error_report() in
addition to returning them.  That's inappropriate.

Two kinds of callers:

1. realize methods (devices "esp" and "virtio-scsi-device")

   The error object gets passed up the call chain until it gets
   reported again and freed.

   Example:

   $ qemu-system-arm -M virt -S -display none \
   > -drive if=scsi,id=foo,bus=1,file=tmp.qcow2 \
   > -device nec-usb-xhci -device usb-storage,drive=foo \
   > -device virtio-scsi-pci
   qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Property 'scsi-disk.drive' can't take value 'foo', it's in use
   qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Setting drive property failed
   qemu-system-arm: -device virtio-scsi-pci: Setting drive property failed
   qemu-system-arm: -device virtio-scsi-pci: Device initialization failed
   qemu-system-arm: -device virtio-scsi-pci: Device 'virtio-scsi-pci' could not be initialized

   The second message in this error cascade comes from
   scsi_bus_legacy_handle_cmdline().  The error object then gets
   passed up to the qdev_init() called from
   virtio_scsi_pci_init_pci(), which reports it again.

2. init methods (devices "am53c974", "dc390", "lsi53c895a",
   "lsi53c810", "megasas", "megasas-gen2", "spapr-vscsi")

   init methods need to report their errors with qerror_report().
   These don't.  The inappropriate error_report() papers over the bug.

   error_report() isn't the same as qerror_report() in QMP context,
   but this can't actually happen: QMP can still only hot-plug, and
   callers call scsi_bus_legacy_handle_cmdline() only on cold-plug.
   Except for sysbus_esp_realize(), but that can't be hot-plugged at
   all, as far as I can tell.

Fix the init methods and drop the inappropriate error_report() in
scsi_bus_legacy_handle_cmdline().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi/esp-pci.c     | 2 ++
 hw/scsi/lsi53c895a.c  | 3 ++-
 hw/scsi/megasas.c     | 2 ++
 hw/scsi/scsi-bus.c    | 1 -
 hw/scsi/spapr_vscsi.c | 2 ++
 5 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index 00b7297..a75fcfa 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -28,6 +28,7 @@
 #include "hw/scsi/esp.h"
 #include "trace.h"
 #include "qemu/log.h"
+#include "qapi/qmp/qerror.h"
 
 #define TYPE_AM53C974_DEVICE "am53c974"
 
@@ -369,6 +370,7 @@ static int esp_pci_scsi_init(PCIDevice *dev)
     if (!d->hotplugged) {
         scsi_bus_legacy_handle_cmdline(&s->bus, &err);
         if (err != NULL) {
+            qerror_report_err(err);
             error_free(err);
             return -1;
         }
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index db7d4b8..4ffab03 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -19,7 +19,7 @@
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/dma.h"
-#include "qemu/error-report.h"
+#include "qapi/qmp/qerror.h"
 
 //#define DEBUG_LSI
 //#define DEBUG_LSI_REG
@@ -2119,6 +2119,7 @@ static int lsi_scsi_init(PCIDevice *dev)
     if (!d->hotplugged) {
         scsi_bus_legacy_handle_cmdline(&s->bus, &err);
         if (err != NULL) {
+            qerror_report_err(err);
             error_free(err);
             return -1;
         }
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 4852237..69ffdbd 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -28,6 +28,7 @@
 #include "hw/scsi/scsi.h"
 #include "block/scsi.h"
 #include "trace.h"
+#include "qapi/qmp/qerror.h"
 
 #include "mfi.h"
 
@@ -2409,6 +2410,7 @@ static int megasas_scsi_init(PCIDevice *dev)
     if (!d->hotplugged) {
         scsi_bus_legacy_handle_cmdline(&s->bus, &err);
         if (err != NULL) {
+            qerror_report_err(err);
             error_free(err);
             return -1;
         }
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index dca9576..f8de569 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -273,7 +273,6 @@ void scsi_bus_legacy_handle_cmdline(SCSIBus *bus, Error **errp)
         scsi_bus_legacy_add_drive(bus, blk_by_legacy_dinfo(dinfo),
                                   unit, false, -1, NULL, &err);
         if (err != NULL) {
-            error_report("%s", error_get_pretty(err));
             error_propagate(errp, err);
             break;
         }
diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index 3639235..f76d334 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -39,6 +39,7 @@
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_vio.h"
 #include "viosrp.h"
+#include "qapi/qmp/qerror.h"
 
 #include <libfdt.h>
 
@@ -1224,6 +1225,7 @@ static int spapr_vscsi_init(VIOsPAPRDevice *dev)
     if (!dev->qdev.hotplugged) {
         scsi_bus_legacy_handle_cmdline(&s->bus, &err);
         if (err != NULL) {
+            qerror_report_err(err);
             error_free(err);
             return -1;
         }
-- 
1.9.3

[Qemu-devel] [PATCH 2/4] hw: Propagate errors through qdev_prop_set_drive()

[Markus Armbruster]

Three kinds of callers:

1. On failure, report the error and abort

   Passing &error_abort does the job.  No functional change.

2. On failure, report the error and exit()

   This is qdev_prop_set_drive_nofail().  Error reporting moves from
   qdev_prop_set_drive() to its caller.  Because hiding away the error
   in the monitor right before exit() isn't helpful, replace
   qerror_report_err() by error_report_err().  Shouldn't make a
   difference, because qdev_prop_set_drive_nofail() should never be
   used in QMP context.

3. On failure, report the error and recover

   This is usb_msd_init() and scsi_bus_legacy_add_drive().  Error
   reporting and freeing the error object moves from
   qdev_prop_set_drive() to its callers.

   Because usb_msd_init() can't run in QMP context, replace
   qerror_report_err() by error_report_err() there.

   No functional change.

   scsi_bus_legacy_add_drive() calling qerror_report_err() is of
   course inappropriate, but this commit merely makes it more obvious.
   The next one will clean it up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/vexpress.c                |  6 +++---
 hw/arm/virt.c                    |  6 +++---
 hw/block/pflash_cfi01.c          |  4 ++--
 hw/block/pflash_cfi02.c          |  4 ++--
 hw/core/qdev-properties-system.c | 22 ++++++++++------------
 hw/scsi/scsi-bus.c               |  6 +++++-
 hw/usb/dev-storage.c             |  7 +++++--
 include/hw/qdev-properties.h     |  4 ++--
 8 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 5933454..8496c16 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -515,9 +515,9 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
 {
     DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
 
-    if (di && qdev_prop_set_drive(dev, "drive",
-                                  blk_by_legacy_dinfo(di))) {
-        abort();
+    if (di) {
+        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di),
+                            &error_abort);
     }
 
     qdev_prop_set_uint32(dev, "num-blocks",
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 69f51ac..93b7605 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -522,9 +522,9 @@ static void create_one_flash(const char *name, hwaddr flashbase,
     DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
     const uint64_t sectorlength = 256 * 1024;
 
-    if (dinfo && qdev_prop_set_drive(dev, "drive",
-                                     blk_by_legacy_dinfo(dinfo))) {
-        abort();
+    if (dinfo) {
+        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo),
+                            &error_abort);
     }
 
     qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength);
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 89d380e..d282695 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -969,8 +969,8 @@ pflash_t *pflash_cfi01_register(hwaddr base,
 {
     DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
 
-    if (blk && qdev_prop_set_drive(dev, "drive", blk)) {
-        abort();
+    if (blk) {
+        qdev_prop_set_drive(dev, "drive", blk, &error_abort);
     }
     qdev_prop_set_uint32(dev, "num-blocks", nb_blocs);
     qdev_prop_set_uint64(dev, "sector-length", sector_len);
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 389b4aa..074a005 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -773,8 +773,8 @@ pflash_t *pflash_cfi02_register(hwaddr base,
 {
     DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH02);
 
-    if (blk && qdev_prop_set_drive(dev, "drive", blk)) {
-        abort();
+    if (blk) {
+        qdev_prop_set_drive(dev, "drive", blk, &error_abort);
     }
     qdev_prop_set_uint32(dev, "num-blocks", nb_blocs);
     qdev_prop_set_uint32(dev, "sector-length", sector_len);
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index a2e44bd..c413226 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -341,27 +341,25 @@ PropertyInfo qdev_prop_vlan = {
     .set   = set_vlan,
 };
 
-int qdev_prop_set_drive(DeviceState *dev, const char *name,
-                        BlockBackend *value)
+void qdev_prop_set_drive(DeviceState *dev, const char *name,
+                         BlockBackend *value, Error **errp)
 {
-    Error *err = NULL;
-    object_property_set_str(OBJECT(dev),
-                            value ? blk_name(value) : "", name, &err);
-    if (err) {
-        qerror_report_err(err);
-        error_free(err);
-        return -1;
-    }
-    return 0;
+    object_property_set_str(OBJECT(dev), value ? blk_name(value) : "",
+                            name, errp);
 }
 
 void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name,
                                 BlockBackend *value)
 {
-    if (qdev_prop_set_drive(dev, name, value) < 0) {
+    Error *err = NULL;
+
+    qdev_prop_set_drive(dev, name, value, &err);
+    if (err) {
+        error_report_err(err);
         exit(1);
     }
 }
+
 void qdev_prop_set_chr(DeviceState *dev, const char *name,
                        CharDriverState *value)
 {
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index f8de569..61c595f 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -7,6 +7,7 @@
 #include "sysemu/blockdev.h"
 #include "trace.h"
 #include "sysemu/dma.h"
+#include "qapi/qmp/qerror.h"
 
 static char *scsibus_get_dev_path(DeviceState *dev);
 static char *scsibus_get_fw_dev_path(DeviceState *dev);
@@ -242,7 +243,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
     if (serial && object_property_find(OBJECT(dev), "serial", NULL)) {
         qdev_prop_set_string(dev, "serial", serial);
     }
-    if (qdev_prop_set_drive(dev, "drive", blk) < 0) {
+    qdev_prop_set_drive(dev, "drive", blk, &err);
+    if (err) {
+        qerror_report_err(err);
+        error_free(err);
         error_setg(errp, "Setting drive property failed");
         object_unparent(OBJECT(dev));
         return NULL;
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index af2e1b9..4c0ef54 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -663,6 +663,7 @@ static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
 static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
 {
     static int nr=0;
+    Error *err = NULL;
     char id[8];
     QemuOpts *opts;
     DriveInfo *dinfo;
@@ -706,8 +707,10 @@ static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
 
     /* create guest device */
     dev = usb_create(bus, "usb-storage");
-    if (qdev_prop_set_drive(&dev->qdev, "drive",
-                            blk_by_legacy_dinfo(dinfo)) < 0) {
+    qdev_prop_set_drive(&dev->qdev, "drive", blk_by_legacy_dinfo(dinfo),
+                        &err);
+    if (err) {
+        error_report_err(err);
         object_unparent(OBJECT(dev));
         return NULL;
     }
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 57ee363..2d47c0c 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -168,8 +168,8 @@ void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value);
 void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value);
 void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value);
 void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value);
-int qdev_prop_set_drive(DeviceState *dev, const char *name,
-                        BlockBackend *value) QEMU_WARN_UNUSED_RESULT;
+void qdev_prop_set_drive(DeviceState *dev, const char *name,
+                         BlockBackend *value, Error **errp);
 void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name,
                                 BlockBackend *value);
 void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value);
-- 
1.9.3

[Qemu-devel] [PATCH 3/4] scsi: Improve error reporting for invalid drive property

[Markus Armbruster]

When setting "realized" fails, scsi_bus_legacy_add_drive() passes the
error to qerror_report_err(), then returns an unspecific "Setting
drive property failed" error, which is reported further up the call
chain.

Example:

    $ qemu-system-x86_64 -nodefaults -S -display none \
    > -drive if=scsi,id=foo,file=tmp.qcow2 -global isa-fdc.driveA=foo
    qemu-system-x86_64: -drive if=scsi,id=foo,file=tmp.qcow2: Property 'scsi-disk.drive' can't take value 'foo', it's in use
    qemu-system-x86_64: Setting drive property failed
    qemu-system-x86_64: Device initialization failed
    qemu-system-x86_64: Initialization of device lsi53c895a failed

Clean up the obvious way: simply return the original error to the
caller.  Gets rid of the second message in the above error cascade.

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

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 61c595f..bd2c0e4 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -7,7 +7,6 @@
 #include "sysemu/blockdev.h"
 #include "trace.h"
 #include "sysemu/dma.h"
-#include "qapi/qmp/qerror.h"
 
 static char *scsibus_get_dev_path(DeviceState *dev);
 static char *scsibus_get_fw_dev_path(DeviceState *dev);
@@ -245,9 +244,7 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
     }
     qdev_prop_set_drive(dev, "drive", blk, &err);
     if (err) {
-        qerror_report_err(err);
-        error_free(err);
-        error_setg(errp, "Setting drive property failed");
+        error_propagate(errp, err);
         object_unparent(OBJECT(dev));
         return NULL;
     }
-- 
1.9.3

[Qemu-devel] [PATCH 4/4] scsi: Convert remaining PCI HBAs to realize()

[Markus Armbruster]

These are "am53c974", "dc390", "lsi53c895a", "lsi53c810", "megasas",
"megasas-gen2".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi/esp-pci.c    | 30 +++++++++++-------------------
 hw/scsi/lsi53c895a.c | 14 +++-----------
 hw/scsi/megasas.c    | 14 +++-----------
 3 files changed, 17 insertions(+), 41 deletions(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index a75fcfa..8d2242d 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -28,7 +28,6 @@
 #include "hw/scsi/esp.h"
 #include "trace.h"
 #include "qemu/log.h"
-#include "qapi/qmp/qerror.h"
 
 #define TYPE_AM53C974_DEVICE "am53c974"
 
@@ -343,13 +342,12 @@ static const struct SCSIBusInfo esp_pci_scsi_info = {
     .cancel = esp_request_cancelled,
 };
 
-static int esp_pci_scsi_init(PCIDevice *dev)
+static void esp_pci_scsi_realize(PCIDevice *dev, Error **errp)
 {
     PCIESPState *pci = PCI_ESP(dev);
     DeviceState *d = DEVICE(dev);
     ESPState *s = &pci->esp;
     uint8_t *pci_conf;
-    Error *err = NULL;
 
     pci_conf = dev->config;
 
@@ -368,14 +366,8 @@ static int esp_pci_scsi_init(PCIDevice *dev)
 
     scsi_bus_new(&s->bus, sizeof(s->bus), d, &esp_pci_scsi_info, NULL);
     if (!d->hotplugged) {
-        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
-        if (err != NULL) {
-            qerror_report_err(err);
-            error_free(err);
-            return -1;
-        }
+        scsi_bus_legacy_handle_cmdline(&s->bus, errp);
     }
-    return 0;
 }
 
 static void esp_pci_scsi_uninit(PCIDevice *d)
@@ -390,7 +382,7 @@ static void esp_pci_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = esp_pci_scsi_init;
+    k->realize = esp_pci_scsi_realize;
     k->exit = esp_pci_scsi_uninit;
     k->vendor_id = PCI_VENDOR_ID_AMD;
     k->device_id = PCI_DEVICE_ID_AMD_SCSI;
@@ -468,17 +460,19 @@ static void dc390_write_config(PCIDevice *dev,
     }
 }
 
-static int dc390_scsi_init(PCIDevice *dev)
+static void dc390_scsi_realize(PCIDevice *dev, Error **errp)
 {
     DC390State *pci = DC390(dev);
+    Error *err = NULL;
     uint8_t *contents;
     uint16_t chksum = 0;
-    int i, ret;
+    int i;
 
     /* init base class */
-    ret = esp_pci_scsi_init(dev);
-    if (ret < 0) {
-        return ret;
+    esp_pci_scsi_realize(dev, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
     }
 
     /* EEPROM */
@@ -505,8 +499,6 @@ static int dc390_scsi_init(PCIDevice *dev)
     chksum = 0x1234 - chksum;
     contents[EE_CHKSUM1] = chksum & 0xff;
     contents[EE_CHKSUM2] = chksum >> 8;
-
-    return 0;
 }
 
 static void dc390_class_init(ObjectClass *klass, void *data)
@@ -514,7 +506,7 @@ static void dc390_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = dc390_scsi_init;
+    k->realize = dc390_scsi_realize;
     k->config_read = dc390_read_config;
     k->config_write = dc390_write_config;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 4ffab03..c5b0cc5 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -19,7 +19,6 @@
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/dma.h"
-#include "qapi/qmp/qerror.h"
 
 //#define DEBUG_LSI
 //#define DEBUG_LSI_REG
@@ -2089,12 +2088,11 @@ static const struct SCSIBusInfo lsi_scsi_info = {
     .cancel = lsi_request_cancelled
 };
 
-static int lsi_scsi_init(PCIDevice *dev)
+static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
 {
     LSIState *s = LSI53C895A(dev);
     DeviceState *d = DEVICE(dev);
     uint8_t *pci_conf;
-    Error *err = NULL;
 
     pci_conf = dev->config;
 
@@ -2117,14 +2115,8 @@ static int lsi_scsi_init(PCIDevice *dev)
 
     scsi_bus_new(&s->bus, sizeof(s->bus), d, &lsi_scsi_info, NULL);
     if (!d->hotplugged) {
-        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
-        if (err != NULL) {
-            qerror_report_err(err);
-            error_free(err);
-            return -1;
-        }
+        scsi_bus_legacy_handle_cmdline(&s->bus, errp);
     }
-    return 0;
 }
 
 static void lsi_class_init(ObjectClass *klass, void *data)
@@ -2132,7 +2124,7 @@ static void lsi_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->init = lsi_scsi_init;
+    k->realize = lsi_scsi_realize;
     k->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
     k->device_id = PCI_DEVICE_ID_LSI_53C895A;
     k->class_id = PCI_CLASS_STORAGE_SCSI;
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 69ffdbd..bf83b65 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -28,7 +28,6 @@
 #include "hw/scsi/scsi.h"
 #include "block/scsi.h"
 #include "trace.h"
-#include "qapi/qmp/qerror.h"
 
 #include "mfi.h"
 
@@ -2321,14 +2320,13 @@ static const struct SCSIBusInfo megasas_scsi_info = {
     .cancel = megasas_command_cancel,
 };
 
-static int megasas_scsi_init(PCIDevice *dev)
+static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
 {
     DeviceState *d = DEVICE(dev);
     MegasasState *s = MEGASAS(dev);
     MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
     uint8_t *pci_conf;
     int i, bar_type;
-    Error *err = NULL;
 
     pci_conf = dev->config;
 
@@ -2408,14 +2406,8 @@ static int megasas_scsi_init(PCIDevice *dev)
     scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
                  &megasas_scsi_info, NULL);
     if (!d->hotplugged) {
-        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
-        if (err != NULL) {
-            qerror_report_err(err);
-            error_free(err);
-            return -1;
-        }
+        scsi_bus_legacy_handle_cmdline(&s->bus, errp);
     }
-    return 0;
 }
 
 static void
@@ -2509,7 +2501,7 @@ static void megasas_class_init(ObjectClass *oc, void *data)
     MegasasBaseClass *e = MEGASAS_DEVICE_CLASS(oc);
     const MegasasInfo *info = data;
 
-    pc->init = megasas_scsi_init;
+    pc->realize = megasas_scsi_realize;
     pc->exit = megasas_scsi_uninit;
     pc->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
     pc->device_id = info->device_id;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/4] scsi: Clean up duplicated error in legacy if=scsi code

[Peter Crosthwaite]

On Fri, Feb 20, 2015 at 11:19 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Commit a818a4b changed scsi_bus_legacy_handle_cmdline() to report
> errors from scsi_bus_legacy_add_drive() with error_report() in
> addition to returning them.  That's inappropriate.
>
> Two kinds of callers:
>
> 1. realize methods (devices "esp" and "virtio-scsi-device")
>
>    The error object gets passed up the call chain until it gets
>    reported again and freed.
>
>    Example:
>
>    $ qemu-system-arm -M virt -S -display none \
>    > -drive if=scsi,id=foo,bus=1,file=tmp.qcow2 \
>    > -device nec-usb-xhci -device usb-storage,drive=foo \
>    > -device virtio-scsi-pci
>    qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Property 'scsi-disk.drive' can't take value 'foo', it's in use
>    qemu-system-arm: -drive if=scsi,id=foo,bus=1,file=tmp.qcow2: Setting drive property failed
>    qemu-system-arm: -device virtio-scsi-pci: Setting drive property failed
>    qemu-system-arm: -device virtio-scsi-pci: Device initialization failed
>    qemu-system-arm: -device virtio-scsi-pci: Device 'virtio-scsi-pci' could not be initialized
>
>    The second message in this error cascade comes from
>    scsi_bus_legacy_handle_cmdline().  The error object then gets
>    passed up to the qdev_init() called from
>    virtio_scsi_pci_init_pci(), which reports it again.
>
> 2. init methods (devices "am53c974", "dc390", "lsi53c895a",
>    "lsi53c810", "megasas", "megasas-gen2", "spapr-vscsi")
>
>    init methods need to report their errors with qerror_report().
>    These don't.  The inappropriate error_report() papers over the bug.
>
>    error_report() isn't the same as qerror_report() in QMP context,
>    but this can't actually happen: QMP can still only hot-plug, and
>    callers call scsi_bus_legacy_handle_cmdline() only on cold-plug.
>    Except for sysbus_esp_realize(), but that can't be hot-plugged at
>    all, as far as I can tell.
>
> Fix the init methods and drop the inappropriate error_report() in
> scsi_bus_legacy_handle_cmdline().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/scsi/esp-pci.c     | 2 ++
>  hw/scsi/lsi53c895a.c  | 3 ++-
>  hw/scsi/megasas.c     | 2 ++
>  hw/scsi/scsi-bus.c    | 1 -
>  hw/scsi/spapr_vscsi.c | 2 ++
>  5 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index 00b7297..a75fcfa 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -28,6 +28,7 @@
>  #include "hw/scsi/esp.h"
>  #include "trace.h"
>  #include "qemu/log.h"
> +#include "qapi/qmp/qerror.h"
>
>  #define TYPE_AM53C974_DEVICE "am53c974"
>
> @@ -369,6 +370,7 @@ static int esp_pci_scsi_init(PCIDevice *dev)
>      if (!d->hotplugged) {
>          scsi_bus_legacy_handle_cmdline(&s->bus, &err);
>          if (err != NULL) {
> +            qerror_report_err(err);
>              error_free(err);
>              return -1;
>          }
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index db7d4b8..4ffab03 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -19,7 +19,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/scsi/scsi.h"
>  #include "sysemu/dma.h"
> -#include "qemu/error-report.h"
> +#include "qapi/qmp/qerror.h"
>
>  //#define DEBUG_LSI
>  //#define DEBUG_LSI_REG
> @@ -2119,6 +2119,7 @@ static int lsi_scsi_init(PCIDevice *dev)
>      if (!d->hotplugged) {
>          scsi_bus_legacy_handle_cmdline(&s->bus, &err);
>          if (err != NULL) {
> +            qerror_report_err(err);
>              error_free(err);
>              return -1;
>          }
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 4852237..69ffdbd 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -28,6 +28,7 @@
>  #include "hw/scsi/scsi.h"
>  #include "block/scsi.h"
>  #include "trace.h"
> +#include "qapi/qmp/qerror.h"
>
>  #include "mfi.h"
>
> @@ -2409,6 +2410,7 @@ static int megasas_scsi_init(PCIDevice *dev)
>      if (!d->hotplugged) {
>          scsi_bus_legacy_handle_cmdline(&s->bus, &err);
>          if (err != NULL) {
> +            qerror_report_err(err);
>              error_free(err);
>              return -1;
>          }
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index dca9576..f8de569 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -273,7 +273,6 @@ void scsi_bus_legacy_handle_cmdline(SCSIBus *bus, Error **errp)
>          scsi_bus_legacy_add_drive(bus, blk_by_legacy_dinfo(dinfo),
>                                    unit, false, -1, NULL, &err);
>          if (err != NULL) {
> -            error_report("%s", error_get_pretty(err));
>              error_propagate(errp, err);
>              break;
>          }
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index 3639235..f76d334 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -39,6 +39,7 @@
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_vio.h"
>  #include "viosrp.h"
> +#include "qapi/qmp/qerror.h"
>
>  #include <libfdt.h>
>
> @@ -1224,6 +1225,7 @@ static int spapr_vscsi_init(VIOsPAPRDevice *dev)
>      if (!dev->qdev.hotplugged) {
>          scsi_bus_legacy_handle_cmdline(&s->bus, &err);
>          if (err != NULL) {
> +            qerror_report_err(err);
>              error_free(err);
>              return -1;
>          }
> --
> 1.9.3
>
>

Re: [Qemu-devel] [PATCH 2/4] hw: Propagate errors through qdev_prop_set_drive()

[Peter Crosthwaite]

On Fri, Feb 20, 2015 at 11:19 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Three kinds of callers:
>
> 1. On failure, report the error and abort
>
>    Passing &error_abort does the job.  No functional change.
>
> 2. On failure, report the error and exit()
>
>    This is qdev_prop_set_drive_nofail().  Error reporting moves from
>    qdev_prop_set_drive() to its caller.  Because hiding away the error
>    in the monitor right before exit() isn't helpful, replace
>    qerror_report_err() by error_report_err().  Shouldn't make a
>    difference, because qdev_prop_set_drive_nofail() should never be
>    used in QMP context.
>
> 3. On failure, report the error and recover
>
>    This is usb_msd_init() and scsi_bus_legacy_add_drive().  Error
>    reporting and freeing the error object moves from
>    qdev_prop_set_drive() to its callers.
>
>    Because usb_msd_init() can't run in QMP context, replace
>    qerror_report_err() by error_report_err() there.
>
>    No functional change.
>
>    scsi_bus_legacy_add_drive() calling qerror_report_err() is of
>    course inappropriate, but this commit merely makes it more obvious.
>    The next one will clean it up.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/arm/vexpress.c                |  6 +++---
>  hw/arm/virt.c                    |  6 +++---
>  hw/block/pflash_cfi01.c          |  4 ++--
>  hw/block/pflash_cfi02.c          |  4 ++--
>  hw/core/qdev-properties-system.c | 22 ++++++++++------------
>  hw/scsi/scsi-bus.c               |  6 +++++-
>  hw/usb/dev-storage.c             |  7 +++++--
>  include/hw/qdev-properties.h     |  4 ++--
>  8 files changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 5933454..8496c16 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -515,9 +515,9 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
>  {
>      DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
>
> -    if (di && qdev_prop_set_drive(dev, "drive",
> -                                  blk_by_legacy_dinfo(di))) {
> -        abort();
> +    if (di) {
> +        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di),
> +                            &error_abort);
>      }
>
>      qdev_prop_set_uint32(dev, "num-blocks",
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 69f51ac..93b7605 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -522,9 +522,9 @@ static void create_one_flash(const char *name, hwaddr flashbase,
>      DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
>      const uint64_t sectorlength = 256 * 1024;
>
> -    if (dinfo && qdev_prop_set_drive(dev, "drive",
> -                                     blk_by_legacy_dinfo(dinfo))) {
> -        abort();
> +    if (dinfo) {
> +        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo),
> +                            &error_abort);
>      }
>
>      qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength);
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 89d380e..d282695 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -969,8 +969,8 @@ pflash_t *pflash_cfi01_register(hwaddr base,
>  {
>      DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
>
> -    if (blk && qdev_prop_set_drive(dev, "drive", blk)) {
> -        abort();
> +    if (blk) {
> +        qdev_prop_set_drive(dev, "drive", blk, &error_abort);
>      }
>      qdev_prop_set_uint32(dev, "num-blocks", nb_blocs);
>      qdev_prop_set_uint64(dev, "sector-length", sector_len);
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 389b4aa..074a005 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -773,8 +773,8 @@ pflash_t *pflash_cfi02_register(hwaddr base,
>  {
>      DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH02);
>
> -    if (blk && qdev_prop_set_drive(dev, "drive", blk)) {
> -        abort();
> +    if (blk) {
> +        qdev_prop_set_drive(dev, "drive", blk, &error_abort);
>      }
>      qdev_prop_set_uint32(dev, "num-blocks", nb_blocs);
>      qdev_prop_set_uint32(dev, "sector-length", sector_len);
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index a2e44bd..c413226 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -341,27 +341,25 @@ PropertyInfo qdev_prop_vlan = {
>      .set   = set_vlan,
>  };
>
> -int qdev_prop_set_drive(DeviceState *dev, const char *name,
> -                        BlockBackend *value)
> +void qdev_prop_set_drive(DeviceState *dev, const char *name,
> +                         BlockBackend *value, Error **errp)
>  {
> -    Error *err = NULL;
> -    object_property_set_str(OBJECT(dev),
> -                            value ? blk_name(value) : "", name, &err);
> -    if (err) {
> -        qerror_report_err(err);
> -        error_free(err);
> -        return -1;
> -    }
> -    return 0;
> +    object_property_set_str(OBJECT(dev), value ? blk_name(value) : "",
> +                            name, errp);
>  }
>
>  void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name,
>                                  BlockBackend *value)
>  {
> -    if (qdev_prop_set_drive(dev, name, value) < 0) {
> +    Error *err = NULL;
> +
> +    qdev_prop_set_drive(dev, name, value, &err);
> +    if (err) {
> +        error_report_err(err);
>          exit(1);
>      }
>  }
> +
>  void qdev_prop_set_chr(DeviceState *dev, const char *name,
>                         CharDriverState *value)
>  {
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index f8de569..61c595f 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -7,6 +7,7 @@
>  #include "sysemu/blockdev.h"
>  #include "trace.h"
>  #include "sysemu/dma.h"
> +#include "qapi/qmp/qerror.h"
>
>  static char *scsibus_get_dev_path(DeviceState *dev);
>  static char *scsibus_get_fw_dev_path(DeviceState *dev);
> @@ -242,7 +243,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
>      if (serial && object_property_find(OBJECT(dev), "serial", NULL)) {
>          qdev_prop_set_string(dev, "serial", serial);
>      }
> -    if (qdev_prop_set_drive(dev, "drive", blk) < 0) {
> +    qdev_prop_set_drive(dev, "drive", blk, &err);
> +    if (err) {
> +        qerror_report_err(err);
> +        error_free(err);
>          error_setg(errp, "Setting drive property failed");
>          object_unparent(OBJECT(dev));
>          return NULL;
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index af2e1b9..4c0ef54 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -663,6 +663,7 @@ static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
>  static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
>  {
>      static int nr=0;
> +    Error *err = NULL;
>      char id[8];
>      QemuOpts *opts;
>      DriveInfo *dinfo;
> @@ -706,8 +707,10 @@ static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
>
>      /* create guest device */
>      dev = usb_create(bus, "usb-storage");
> -    if (qdev_prop_set_drive(&dev->qdev, "drive",
> -                            blk_by_legacy_dinfo(dinfo)) < 0) {
> +    qdev_prop_set_drive(&dev->qdev, "drive", blk_by_legacy_dinfo(dinfo),
> +                        &err);
> +    if (err) {
> +        error_report_err(err);
>          object_unparent(OBJECT(dev));
>          return NULL;
>      }
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 57ee363..2d47c0c 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -168,8 +168,8 @@ void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value);
>  void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value);
>  void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value);
>  void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value);
> -int qdev_prop_set_drive(DeviceState *dev, const char *name,
> -                        BlockBackend *value) QEMU_WARN_UNUSED_RESULT;
> +void qdev_prop_set_drive(DeviceState *dev, const char *name,
> +                         BlockBackend *value, Error **errp);
>  void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name,
>                                  BlockBackend *value);
>  void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value);
> --
> 1.9.3
>
>

Re: [Qemu-devel] [PATCH 3/4] scsi: Improve error reporting for invalid drive property

[Peter Crosthwaite]

On Fri, Feb 20, 2015 at 11:19 AM, Markus Armbruster <armbru@redhat.com> wrote:
> When setting "realized" fails, scsi_bus_legacy_add_drive() passes the
> error to qerror_report_err(), then returns an unspecific "Setting
> drive property failed" error, which is reported further up the call
> chain.
>
> Example:
>
>     $ qemu-system-x86_64 -nodefaults -S -display none \
>     > -drive if=scsi,id=foo,file=tmp.qcow2 -global isa-fdc.driveA=foo
>     qemu-system-x86_64: -drive if=scsi,id=foo,file=tmp.qcow2: Property 'scsi-disk.drive' can't take value 'foo', it's in use
>     qemu-system-x86_64: Setting drive property failed
>     qemu-system-x86_64: Device initialization failed
>     qemu-system-x86_64: Initialization of device lsi53c895a failed
>
> Clean up the obvious way: simply return the original error to the
> caller.  Gets rid of the second message in the above error cascade.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/scsi/scsi-bus.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 61c595f..bd2c0e4 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -7,7 +7,6 @@
>  #include "sysemu/blockdev.h"
>  #include "trace.h"
>  #include "sysemu/dma.h"
> -#include "qapi/qmp/qerror.h"
>
>  static char *scsibus_get_dev_path(DeviceState *dev);
>  static char *scsibus_get_fw_dev_path(DeviceState *dev);
> @@ -245,9 +244,7 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
>      }
>      qdev_prop_set_drive(dev, "drive", blk, &err);
>      if (err) {
> -        qerror_report_err(err);
> -        error_free(err);
> -        error_setg(errp, "Setting drive property failed");
> +        error_propagate(errp, err);
>          object_unparent(OBJECT(dev));
>          return NULL;
>      }
> --
> 1.9.3
>
>

Re: [Qemu-devel] [PATCH 4/4] scsi: Convert remaining PCI HBAs to realize()

[Peter Crosthwaite]

On Fri, Feb 20, 2015 at 11:20 AM, Markus Armbruster <armbru@redhat.com> wrote:
> These are "am53c974", "dc390", "lsi53c895a", "lsi53c810", "megasas",
> "megasas-gen2".
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/scsi/esp-pci.c    | 30 +++++++++++-------------------
>  hw/scsi/lsi53c895a.c | 14 +++-----------
>  hw/scsi/megasas.c    | 14 +++-----------
>  3 files changed, 17 insertions(+), 41 deletions(-)
>
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index a75fcfa..8d2242d 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -28,7 +28,6 @@
>  #include "hw/scsi/esp.h"
>  #include "trace.h"
>  #include "qemu/log.h"
> -#include "qapi/qmp/qerror.h"
>
>  #define TYPE_AM53C974_DEVICE "am53c974"
>
> @@ -343,13 +342,12 @@ static const struct SCSIBusInfo esp_pci_scsi_info = {
>      .cancel = esp_request_cancelled,
>  };
>
> -static int esp_pci_scsi_init(PCIDevice *dev)
> +static void esp_pci_scsi_realize(PCIDevice *dev, Error **errp)
>  {
>      PCIESPState *pci = PCI_ESP(dev);
>      DeviceState *d = DEVICE(dev);
>      ESPState *s = &pci->esp;
>      uint8_t *pci_conf;
> -    Error *err = NULL;
>
>      pci_conf = dev->config;
>
> @@ -368,14 +366,8 @@ static int esp_pci_scsi_init(PCIDevice *dev)
>
>      scsi_bus_new(&s->bus, sizeof(s->bus), d, &esp_pci_scsi_info, NULL);
>      if (!d->hotplugged) {
> -        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
> -        if (err != NULL) {
> -            qerror_report_err(err);
> -            error_free(err);
> -            return -1;
> -        }
> +        scsi_bus_legacy_handle_cmdline(&s->bus, errp);
>      }
> -    return 0;
>  }
>
>  static void esp_pci_scsi_uninit(PCIDevice *d)
> @@ -390,7 +382,7 @@ static void esp_pci_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -    k->init = esp_pci_scsi_init;
> +    k->realize = esp_pci_scsi_realize;
>      k->exit = esp_pci_scsi_uninit;
>      k->vendor_id = PCI_VENDOR_ID_AMD;
>      k->device_id = PCI_DEVICE_ID_AMD_SCSI;
> @@ -468,17 +460,19 @@ static void dc390_write_config(PCIDevice *dev,
>      }
>  }
>
> -static int dc390_scsi_init(PCIDevice *dev)
> +static void dc390_scsi_realize(PCIDevice *dev, Error **errp)
>  {
>      DC390State *pci = DC390(dev);
> +    Error *err = NULL;
>      uint8_t *contents;
>      uint16_t chksum = 0;
> -    int i, ret;
> +    int i;
>
>      /* init base class */
> -    ret = esp_pci_scsi_init(dev);
> -    if (ret < 0) {
> -        return ret;
> +    esp_pci_scsi_realize(dev, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
>      }
>
>      /* EEPROM */
> @@ -505,8 +499,6 @@ static int dc390_scsi_init(PCIDevice *dev)
>      chksum = 0x1234 - chksum;
>      contents[EE_CHKSUM1] = chksum & 0xff;
>      contents[EE_CHKSUM2] = chksum >> 8;
> -
> -    return 0;
>  }
>
>  static void dc390_class_init(ObjectClass *klass, void *data)
> @@ -514,7 +506,7 @@ static void dc390_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -    k->init = dc390_scsi_init;
> +    k->realize = dc390_scsi_realize;
>      k->config_read = dc390_read_config;
>      k->config_write = dc390_write_config;
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index 4ffab03..c5b0cc5 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -19,7 +19,6 @@
>  #include "hw/pci/pci.h"
>  #include "hw/scsi/scsi.h"
>  #include "sysemu/dma.h"
> -#include "qapi/qmp/qerror.h"
>
>  //#define DEBUG_LSI
>  //#define DEBUG_LSI_REG
> @@ -2089,12 +2088,11 @@ static const struct SCSIBusInfo lsi_scsi_info = {
>      .cancel = lsi_request_cancelled
>  };
>
> -static int lsi_scsi_init(PCIDevice *dev)
> +static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
>  {
>      LSIState *s = LSI53C895A(dev);
>      DeviceState *d = DEVICE(dev);
>      uint8_t *pci_conf;
> -    Error *err = NULL;
>
>      pci_conf = dev->config;
>
> @@ -2117,14 +2115,8 @@ static int lsi_scsi_init(PCIDevice *dev)
>
>      scsi_bus_new(&s->bus, sizeof(s->bus), d, &lsi_scsi_info, NULL);
>      if (!d->hotplugged) {
> -        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
> -        if (err != NULL) {
> -            qerror_report_err(err);
> -            error_free(err);
> -            return -1;
> -        }
> +        scsi_bus_legacy_handle_cmdline(&s->bus, errp);
>      }
> -    return 0;
>  }
>
>  static void lsi_class_init(ObjectClass *klass, void *data)
> @@ -2132,7 +2124,7 @@ static void lsi_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> -    k->init = lsi_scsi_init;
> +    k->realize = lsi_scsi_realize;
>      k->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
>      k->device_id = PCI_DEVICE_ID_LSI_53C895A;
>      k->class_id = PCI_CLASS_STORAGE_SCSI;
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 69ffdbd..bf83b65 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -28,7 +28,6 @@
>  #include "hw/scsi/scsi.h"
>  #include "block/scsi.h"
>  #include "trace.h"
> -#include "qapi/qmp/qerror.h"
>
>  #include "mfi.h"
>
> @@ -2321,14 +2320,13 @@ static const struct SCSIBusInfo megasas_scsi_info = {
>      .cancel = megasas_command_cancel,
>  };
>
> -static int megasas_scsi_init(PCIDevice *dev)
> +static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>  {
>      DeviceState *d = DEVICE(dev);
>      MegasasState *s = MEGASAS(dev);
>      MegasasBaseClass *b = MEGASAS_DEVICE_GET_CLASS(s);
>      uint8_t *pci_conf;
>      int i, bar_type;
> -    Error *err = NULL;
>
>      pci_conf = dev->config;
>
> @@ -2408,14 +2406,8 @@ static int megasas_scsi_init(PCIDevice *dev)
>      scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
>                   &megasas_scsi_info, NULL);
>      if (!d->hotplugged) {
> -        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
> -        if (err != NULL) {
> -            qerror_report_err(err);
> -            error_free(err);
> -            return -1;
> -        }
> +        scsi_bus_legacy_handle_cmdline(&s->bus, errp);
>      }
> -    return 0;
>  }
>
>  static void
> @@ -2509,7 +2501,7 @@ static void megasas_class_init(ObjectClass *oc, void *data)
>      MegasasBaseClass *e = MEGASAS_DEVICE_CLASS(oc);
>      const MegasasInfo *info = data;
>
> -    pc->init = megasas_scsi_init;
> +    pc->realize = megasas_scsi_realize;
>      pc->exit = megasas_scsi_uninit;
>      pc->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
>      pc->device_id = info->device_id;
> --
> 1.9.3
>
>