[Qemu-devel] [PATCH v3 0/2] scsi: Change device init to realize

[Qemu-devel] [PATCH v3 0/2] scsi: Change device init to realize

[Fam Zheng]

v3: Fix error handling of scsi_device_init (Kevin)
    Thanks for reviewing!

DeviceClass->init is the old interface, let's convert scsi devices to the new
->realize API.

A user visible change is the error message shown in qemu-iotests reference
output, but I don't know what's the best way to retain it. So please review if
and how this could be improved.

Fam


Fam Zheng (2):
  block: Pass errp in blkconf_geometry
  scsi-bus: Convert DeviceClass init to realize

 hw/block/block.c           | 18 +++++-----
 hw/block/virtio-blk.c      |  7 ++--
 hw/ide/qdev.c              | 11 ++++--
 hw/scsi/lsi53c895a.c       |  2 ++
 hw/scsi/scsi-bus.c         | 69 ++++++++++++++++++--------------------
 hw/scsi/scsi-disk.c        | 83 +++++++++++++++++++++++++---------------------
 hw/scsi/scsi-generic.c     | 37 ++++++++++-----------
 include/hw/block/block.h   |  6 ++--
 include/hw/scsi/scsi.h     |  7 ++--
 tests/qemu-iotests/051.out |  4 +--
 10 files changed, 128 insertions(+), 116 deletions(-)

-- 
2.0.3

[Qemu-devel] [PATCH v3 1/2] block: Pass errp in blkconf_geometry

[Fam Zheng]

This allows us to pass error information to caller.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/block.c         | 18 +++++++++---------
 hw/block/virtio-blk.c    |  7 +++----
 hw/ide/qdev.c            | 11 ++++++++---
 hw/scsi/scsi-disk.c      | 11 ++++++++---
 include/hw/block/block.h |  6 ++++--
 5 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index 33dd3f3..b6a6dc6 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -22,8 +22,9 @@ void blkconf_serial(BlockConf *conf, char **serial)
     }
 }
 
-int blkconf_geometry(BlockConf *conf, int *ptrans,
-                     unsigned cyls_max, unsigned heads_max, unsigned secs_max)
+void blkconf_geometry(BlockConf *conf, int *ptrans,
+                      unsigned cyls_max, unsigned heads_max, unsigned secs_max,
+                      Error **errp)
 {
     DriveInfo *dinfo;
 
@@ -46,17 +47,16 @@ int blkconf_geometry(BlockConf *conf, int *ptrans,
     }
     if (conf->cyls || conf->heads || conf->secs) {
         if (conf->cyls < 1 || conf->cyls > cyls_max) {
-            error_report("cyls must be between 1 and %u", cyls_max);
-            return -1;
+            error_setg(errp, "cyls must be between 1 and %u", cyls_max);
+            return;
         }
         if (conf->heads < 1 || conf->heads > heads_max) {
-            error_report("heads must be between 1 and %u", heads_max);
-            return -1;
+            error_setg(errp, "heads must be between 1 and %u", heads_max);
+            return;
         }
         if (conf->secs < 1 || conf->secs > secs_max) {
-            error_report("secs must be between 1 and %u", secs_max);
-            return -1;
+            error_setg(errp, "secs must be between 1 and %u", secs_max);
+            return;
         }
     }
-    return 0;
 }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index c241c50..02106ce 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -728,9 +728,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBlock *s = VIRTIO_BLK(dev);
     VirtIOBlkConf *blk = &(s->blk);
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     Error *err = NULL;
-#endif
     static int virtio_blk_id;
 
     if (!blk->conf.bs) {
@@ -744,8 +742,9 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
 
     blkconf_serial(&blk->conf, &blk->serial);
     s->original_wce = bdrv_enable_write_cache(blk->conf.bs);
-    if (blkconf_geometry(&blk->conf, NULL, 65535, 255, 255) < 0) {
-        error_setg(errp, "Error setting geometry");
+    blkconf_geometry(&blk->conf, NULL, 65535, 255, 255, &err);
+    if (err) {
+        error_propagate(errp, err);
         return;
     }
 
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6e475e6..b4a4671 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -151,6 +151,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
 {
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
     IDEState *s = bus->ifs + dev->unit;
+    Error *err = NULL;
 
     if (dev->conf.discard_granularity == -1) {
         dev->conf.discard_granularity = 512;
@@ -161,9 +162,13 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
     }
 
     blkconf_serial(&dev->conf, &dev->serial);
-    if (kind != IDE_CD
-        && blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255) < 0) {
-        return -1;
+    if (kind != IDE_CD) {
+        blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err);
+        if (err) {
+            error_report("%s", error_get_pretty(err));
+            error_free(err);
+            return -1;
+        }
     }
 
     if (ide_init_drive(s, dev->conf.bs, kind,
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index d47ecd6..9010724 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2237,6 +2237,7 @@ static void scsi_disk_unit_attention_reported(SCSIDevice *dev)
 static int scsi_initfn(SCSIDevice *dev)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+    Error *err = NULL;
 
     if (!s->qdev.conf.bs) {
         error_report("drive property not set");
@@ -2250,9 +2251,13 @@ static int scsi_initfn(SCSIDevice *dev)
     }
 
     blkconf_serial(&s->qdev.conf, &s->serial);
-    if (dev->type == TYPE_DISK
-        && blkconf_geometry(&dev->conf, NULL, 65535, 255, 255) < 0) {
-        return -1;
+    if (dev->type == TYPE_DISK) {
+        blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err);
+        if (err) {
+            error_report("%s", error_get_pretty(err));
+            error_free(err);
+            return -1;
+        }
     }
 
     if (s->qdev.conf.discard_granularity == -1) {
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 7c3d6c8..3a01488 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -12,6 +12,7 @@
 #define HW_BLOCK_COMMON_H
 
 #include "qemu-common.h"
+#include "qapi/error.h"
 
 /* Configuration */
 
@@ -60,8 +61,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
 /* Configuration helpers */
 
 void blkconf_serial(BlockConf *conf, char **serial);
-int blkconf_geometry(BlockConf *conf, int *trans,
-                     unsigned cyls_max, unsigned heads_max, unsigned secs_max);
+void blkconf_geometry(BlockConf *conf, int *trans,
+                      unsigned cyls_max, unsigned heads_max, unsigned secs_max,
+                      Error **errp);
 
 /* Hard disk geometry */
 
-- 
2.0.3

[Qemu-devel] [PATCH v3 2/2] scsi-bus: Convert DeviceClass init to realize

[Fam Zheng]

Replace "init/destroy" with "realize/unrealize" in SCSIDeviceClass,
which has errp as a parameter. So all the implementations now use
error_setg instead of error_report for reporting error.

Also in lsi53c895a, report the error when initializing the if=scsi
devices, before dropping it, because in the callee, error_report is
changed to error_setg.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/lsi53c895a.c       |  2 ++
 hw/scsi/scsi-bus.c         | 69 ++++++++++++++++++++--------------------
 hw/scsi/scsi-disk.c        | 78 ++++++++++++++++++++++++----------------------
 hw/scsi/scsi-generic.c     | 37 +++++++++++-----------
 include/hw/scsi/scsi.h     |  7 +++--
 tests/qemu-iotests/051.out |  4 +--
 6 files changed, 99 insertions(+), 98 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 786d848..dbc98a0 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -19,6 +19,7 @@
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/dma.h"
+#include "qemu/error-report.h"
 
 //#define DEBUG_LSI
 //#define DEBUG_LSI_REG
@@ -2121,6 +2122,7 @@ static int lsi_scsi_init(PCIDevice *dev)
     if (!d->hotplugged) {
         scsi_bus_legacy_handle_cmdline(&s->bus, &err);
         if (err != NULL) {
+            error_report("%s", error_get_pretty(err));
             error_free(err);
             return -1;
         }
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 4341754..da1276e 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -37,20 +37,19 @@ static const TypeInfo scsi_bus_info = {
 };
 static int next_scsi_bus;
 
-static int scsi_device_init(SCSIDevice *s)
+static void scsi_device_realize(SCSIDevice *s, Error **errp)
 {
     SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
-    if (sc->init) {
-        return sc->init(s);
+    if (sc->realize) {
+        sc->realize(s, errp);
     }
-    return 0;
 }
 
-static void scsi_device_destroy(SCSIDevice *s)
+static void scsi_device_unrealize(SCSIDevice *s, Error **errp)
 {
     SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
-    if (sc->destroy) {
-        sc->destroy(s);
+    if (sc->unrealize) {
+        sc->unrealize(s, errp);
     }
 }
 
@@ -130,24 +129,24 @@ static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
     }
 }
 
-static int scsi_qdev_init(DeviceState *qdev)
+static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
 {
     SCSIDevice *dev = SCSI_DEVICE(qdev);
     SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
     SCSIDevice *d;
-    int rc = -1;
+    Error *local_err = NULL;
 
     if (dev->channel > bus->info->max_channel) {
-        error_report("bad scsi channel id: %d", dev->channel);
-        goto err;
+        error_setg(errp, "bad scsi channel id: %d", dev->channel);
+        return;
     }
     if (dev->id != -1 && dev->id > bus->info->max_target) {
-        error_report("bad scsi device id: %d", dev->id);
-        goto err;
+        error_setg(errp, "bad scsi device id: %d", dev->id);
+        return;
     }
     if (dev->lun != -1 && dev->lun > bus->info->max_lun) {
-        error_report("bad scsi device lun: %d", dev->lun);
-        goto err;
+        error_setg(errp, "bad scsi device lun: %d", dev->lun);
+        return;
     }
 
     if (dev->id == -1) {
@@ -159,8 +158,8 @@ static int scsi_qdev_init(DeviceState *qdev)
             d = scsi_device_find(bus, dev->channel, ++id, dev->lun);
         } while (d && d->lun == dev->lun && id < bus->info->max_target);
         if (d && d->lun == dev->lun) {
-            error_report("no free target");
-            goto err;
+            error_setg(errp, "no free target");
+            return;
         }
         dev->id = id;
     } else if (dev->lun == -1) {
@@ -169,43 +168,41 @@ static int scsi_qdev_init(DeviceState *qdev)
             d = scsi_device_find(bus, dev->channel, dev->id, ++lun);
         } while (d && d->lun == lun && lun < bus->info->max_lun);
         if (d && d->lun == lun) {
-            error_report("no free lun");
-            goto err;
+            error_setg(errp, "no free lun");
+            return;
         }
         dev->lun = lun;
     } else {
         d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);
         assert(d);
         if (d->lun == dev->lun && dev != d) {
-            error_report("lun already used by '%s'", d->qdev.id);
-            goto err;
+            error_setg(errp, "lun already used by '%s'", d->qdev.id);
+            return;
         }
     }
 
     QTAILQ_INIT(&dev->requests);
-    rc = scsi_device_init(dev);
-    if (rc == 0) {
-        dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
-                                                         dev);
+    scsi_device_realize(dev, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
     }
+    dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
+                                                     dev);
 
     if (bus->info->hotplug) {
         bus->info->hotplug(bus, dev);
     }
-
-err:
-    return rc;
 }
 
-static int scsi_qdev_exit(DeviceState *qdev)
+static void scsi_qdev_unrealize(DeviceState *qdev, Error **errp)
 {
     SCSIDevice *dev = SCSI_DEVICE(qdev);
 
     if (dev->vmsentry) {
         qemu_del_vm_change_state_handler(dev->vmsentry);
     }
-    scsi_device_destroy(dev);
-    return 0;
+    scsi_device_unrealize(dev, errp);
 }
 
 /* handle legacy '-drive if=scsi,...' cmd line args */
@@ -1964,11 +1961,11 @@ static void scsi_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
     set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
-    k->bus_type = TYPE_SCSI_BUS;
-    k->init     = scsi_qdev_init;
-    k->unplug   = scsi_qdev_unplug;
-    k->exit     = scsi_qdev_exit;
-    k->props    = scsi_props;
+    k->bus_type  = TYPE_SCSI_BUS;
+    k->realize   = scsi_qdev_realize;
+    k->unplug    = scsi_qdev_unplug;
+    k->unrealize = scsi_qdev_unrealize;
+    k->props     = scsi_props;
 }
 
 static const TypeInfo scsi_device_type_info = {
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 9010724..5e634b3 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2151,7 +2151,7 @@ static void scsi_disk_reset(DeviceState *dev)
     s->tray_open = 0;
 }
 
-static void scsi_destroy(SCSIDevice *dev)
+static void scsi_unrealize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
 
@@ -2234,29 +2234,28 @@ static void scsi_disk_unit_attention_reported(SCSIDevice *dev)
     }
 }
 
-static int scsi_initfn(SCSIDevice *dev)
+static void scsi_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
     Error *err = NULL;
 
     if (!s->qdev.conf.bs) {
-        error_report("drive property not set");
-        return -1;
+        error_setg(errp, "drive property not set");
+        return;
     }
 
     if (!(s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
         !bdrv_is_inserted(s->qdev.conf.bs)) {
-        error_report("Device needs media, but drive is empty");
-        return -1;
+        error_setg(errp, "Device needs media, but drive is empty");
+        return;
     }
 
     blkconf_serial(&s->qdev.conf, &s->serial);
     if (dev->type == TYPE_DISK) {
         blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err);
         if (err) {
-            error_report("%s", error_get_pretty(err));
-            error_free(err);
-            return -1;
+            error_propagate(errp, err);
+            return;
         }
     }
 
@@ -2273,8 +2272,8 @@ static int scsi_initfn(SCSIDevice *dev)
     }
 
     if (bdrv_is_sg(s->qdev.conf.bs)) {
-        error_report("unwanted /dev/sg*");
-        return -1;
+        error_setg(errp, "unwanted /dev/sg*");
+        return;
     }
 
     if ((s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
@@ -2287,10 +2286,9 @@ static int scsi_initfn(SCSIDevice *dev)
 
     bdrv_iostatus_enable(s->qdev.conf.bs);
     add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, NULL);
-    return 0;
 }
 
-static int scsi_hd_initfn(SCSIDevice *dev)
+static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
     s->qdev.blocksize = s->qdev.conf.logical_block_size;
@@ -2298,10 +2296,10 @@ static int scsi_hd_initfn(SCSIDevice *dev)
     if (!s->product) {
         s->product = g_strdup("QEMU HARDDISK");
     }
-    return scsi_initfn(&s->qdev);
+    scsi_realize(&s->qdev, errp);
 }
 
-static int scsi_cd_initfn(SCSIDevice *dev)
+static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
     s->qdev.blocksize = 2048;
@@ -2310,22 +2308,26 @@ static int scsi_cd_initfn(SCSIDevice *dev)
     if (!s->product) {
         s->product = g_strdup("QEMU CD-ROM");
     }
-    return scsi_initfn(&s->qdev);
+    scsi_realize(&s->qdev, errp);
 }
 
-static int scsi_disk_initfn(SCSIDevice *dev)
+static void scsi_disk_realize(SCSIDevice *dev, Error **errp)
 {
     DriveInfo *dinfo;
+    Error *local_err = NULL;
 
     if (!dev->conf.bs) {
-        return scsi_initfn(dev);  /* ... and die there */
+        scsi_realize(dev, &local_err);
+        assert(local_err);
+        error_propagate(errp, local_err);
+        return;
     }
 
     dinfo = drive_get_by_blockdev(dev->conf.bs);
     if (dinfo->media_cd) {
-        return scsi_cd_initfn(dev);
+        scsi_cd_realize(dev, errp);
     } else {
-        return scsi_hd_initfn(dev);
+        scsi_hd_realize(dev, errp);
     }
 }
 
@@ -2457,35 +2459,35 @@ static int get_device_type(SCSIDiskState *s)
     return 0;
 }
 
-static int scsi_block_initfn(SCSIDevice *dev)
+static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
     int sg_version;
     int rc;
 
     if (!s->qdev.conf.bs) {
-        error_report("drive property not set");
-        return -1;
+        error_setg(errp, "drive property not set");
+        return;
     }
 
     /* check we are using a driver managing SG_IO (version 3 and after) */
     rc = bdrv_ioctl(s->qdev.conf.bs, SG_GET_VERSION_NUM, &sg_version);
     if (rc < 0) {
-        error_report("cannot get SG_IO version number: %s.  "
+        error_setg(errp, "cannot get SG_IO version number: %s.  "
                      "Is this a SCSI device?",
                      strerror(-rc));
-        return -1;
+        return;
     }
     if (sg_version < 30000) {
-        error_report("scsi generic interface too old");
-        return -1;
+        error_setg(errp, "scsi generic interface too old");
+        return;
     }
 
     /* get device type from INQUIRY data */
     rc = get_device_type(s);
     if (rc < 0) {
-        error_report("INQUIRY failed");
-        return -1;
+        error_setg(errp, "INQUIRY failed");
+        return;
     }
 
     /* Make a guess for the block size, we'll fix it when the guest sends.
@@ -2503,7 +2505,7 @@ static int scsi_block_initfn(SCSIDevice *dev)
      */
     s->features |= (1 << SCSI_DISK_F_NO_REMOVABLE_DEVOPS);
 
-    return scsi_initfn(&s->qdev);
+    scsi_realize(&s->qdev, errp);
 }
 
 static SCSIRequest *scsi_block_new_request(SCSIDevice *d, uint32_t tag,
@@ -2599,8 +2601,8 @@ static void scsi_hd_class_initfn(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     SCSIDeviceClass *sc = SCSI_DEVICE_CLASS(klass);
 
-    sc->init         = scsi_hd_initfn;
-    sc->destroy      = scsi_destroy;
+    sc->realize      = scsi_hd_realize;
+    sc->unrealize    = scsi_unrealize;
     sc->alloc_req    = scsi_new_request;
     sc->unit_attention_reported = scsi_disk_unit_attention_reported;
     dc->fw_name = "disk";
@@ -2630,8 +2632,8 @@ static void scsi_cd_class_initfn(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     SCSIDeviceClass *sc = SCSI_DEVICE_CLASS(klass);
 
-    sc->init         = scsi_cd_initfn;
-    sc->destroy      = scsi_destroy;
+    sc->realize      = scsi_cd_realize;
+    sc->unrealize    = scsi_unrealize;
     sc->alloc_req    = scsi_new_request;
     sc->unit_attention_reported = scsi_disk_unit_attention_reported;
     dc->fw_name = "disk";
@@ -2660,8 +2662,8 @@ static void scsi_block_class_initfn(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     SCSIDeviceClass *sc = SCSI_DEVICE_CLASS(klass);
 
-    sc->init         = scsi_block_initfn;
-    sc->destroy      = scsi_destroy;
+    sc->realize      = scsi_block_realize;
+    sc->unrealize    = scsi_unrealize;
     sc->alloc_req    = scsi_block_new_request;
     dc->fw_name = "disk";
     dc->desc = "SCSI block device passthrough";
@@ -2697,8 +2699,8 @@ static void scsi_disk_class_initfn(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     SCSIDeviceClass *sc = SCSI_DEVICE_CLASS(klass);
 
-    sc->init         = scsi_disk_initfn;
-    sc->destroy      = scsi_destroy;
+    sc->realize      = scsi_disk_realize;
+    sc->unrealize    = scsi_unrealize;
     sc->alloc_req    = scsi_new_request;
     sc->unit_attention_reported = scsi_disk_unit_attention_reported;
     dc->fw_name = "disk";
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 3733d2c..c34a0bc 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -386,49 +386,49 @@ static void scsi_generic_reset(DeviceState *dev)
     scsi_device_purge_requests(s, SENSE_CODE(RESET));
 }
 
-static void scsi_destroy(SCSIDevice *s)
+static void scsi_unrealize(SCSIDevice *s, Error **errp)
 {
     scsi_device_purge_requests(s, SENSE_CODE(NO_SENSE));
     blockdev_mark_auto_del(s->conf.bs);
 }
 
-static int scsi_generic_initfn(SCSIDevice *s)
+static void scsi_generic_realize(SCSIDevice *s, Error **errp)
 {
     int rc;
     int sg_version;
     struct sg_scsi_id scsiid;
 
     if (!s->conf.bs) {
-        error_report("drive property not set");
-        return -1;
+        error_setg(errp, "drive property not set");
+        return;
     }
 
     if (bdrv_get_on_error(s->conf.bs, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
-        error_report("Device doesn't support drive option werror");
-        return -1;
+        error_setg(errp, "Device doesn't support drive option werror");
+        return;
     }
     if (bdrv_get_on_error(s->conf.bs, 1) != BLOCKDEV_ON_ERROR_REPORT) {
-        error_report("Device doesn't support drive option rerror");
-        return -1;
+        error_setg(errp, "Device doesn't support drive option rerror");
+        return;
     }
 
     /* check we are using a driver managing SG_IO (version 3 and after */
     rc = bdrv_ioctl(s->conf.bs, SG_GET_VERSION_NUM, &sg_version);
     if (rc < 0) {
-        error_report("cannot get SG_IO version number: %s.  "
-                     "Is this a SCSI device?",
-                     strerror(-rc));
-        return -1;
+        error_setg(errp, "cannot get SG_IO version number: %s.  "
+                         "Is this a SCSI device?",
+                         strerror(-rc));
+        return;
     }
     if (sg_version < 30000) {
-        error_report("scsi generic interface too old");
-        return -1;
+        error_setg(errp, "scsi generic interface too old");
+        return;
     }
 
     /* get LUN of the /dev/sg? */
     if (bdrv_ioctl(s->conf.bs, SG_GET_SCSI_ID, &scsiid)) {
-        error_report("SG_GET_SCSI_ID ioctl failed");
-        return -1;
+        error_setg(errp, "SG_GET_SCSI_ID ioctl failed");
+        return;
     }
 
     /* define device state */
@@ -460,7 +460,6 @@ static int scsi_generic_initfn(SCSIDevice *s)
     }
 
     DPRINTF("block size %d\n", s->blocksize);
-    return 0;
 }
 
 const SCSIReqOps scsi_generic_req_ops = {
@@ -495,8 +494,8 @@ static void scsi_generic_class_initfn(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     SCSIDeviceClass *sc = SCSI_DEVICE_CLASS(klass);
 
-    sc->init         = scsi_generic_initfn;
-    sc->destroy      = scsi_destroy;
+    sc->realize      = scsi_generic_realize;
+    sc->unrealize    = scsi_unrealize;
     sc->alloc_req    = scsi_new_request;
     dc->fw_name = "disk";
     dc->desc = "pass through generic scsi device (/dev/sg*)";
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 1adb549..d64ef24 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -72,10 +72,13 @@ struct SCSIRequest {
 #define SCSI_DEVICE_GET_CLASS(obj) \
      OBJECT_GET_CLASS(SCSIDeviceClass, (obj), TYPE_SCSI_DEVICE)
 
+typedef void (*SCSIDeviceRealize)(SCSIDevice *dev, Error **errp);
+typedef void (*SCSIDeviceUnrealize)(SCSIDevice *dev, Error **errp);
+
 typedef struct SCSIDeviceClass {
     DeviceClass parent_class;
-    int (*init)(SCSIDevice *dev);
-    void (*destroy)(SCSIDevice *s);
+    SCSIDeviceRealize realize;
+    SCSIDeviceUnrealize unrealize;
     SCSIRequest *(*alloc_req)(SCSIDevice *s, uint32_t tag, uint32_t lun,
                               uint8_t *buf, void *hba_private);
     void (*unit_attention_reported)(SCSIDevice *s);
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index d7b0f50..f6d9dc1 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -122,7 +122,7 @@ QEMU_PROG: -drive if=virtio: Device 'virtio-blk-pci' could not be initialized
 
 Testing: -drive if=scsi
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -drive if=scsi: Device needs media, but drive is empty
+(qemu) QEMU_PROG: Device needs media, but drive is empty
 QEMU_PROG: Device initialization failed.
 QEMU_PROG: Initialization of device lsi53c895a failed
 
@@ -149,13 +149,11 @@ QEMU_PROG: -device ide-hd,drive=disk: Device 'ide-hd' could not be initialized
 Testing: -drive if=none,id=disk -device lsi53c895a -device scsi-disk,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) QEMU_PROG: -device scsi-disk,drive=disk: Device needs media, but drive is empty
-QEMU_PROG: -device scsi-disk,drive=disk: Device initialization failed.
 QEMU_PROG: -device scsi-disk,drive=disk: Device 'scsi-disk' could not be initialized
 
 Testing: -drive if=none,id=disk -device lsi53c895a -device scsi-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) QEMU_PROG: -device scsi-hd,drive=disk: Device needs media, but drive is empty
-QEMU_PROG: -device scsi-hd,drive=disk: Device initialization failed.
 QEMU_PROG: -device scsi-hd,drive=disk: Device 'scsi-hd' could not be initialized
 
 
-- 
2.0.3

Re: [Qemu-devel] [PATCH v3 1/2] block: Pass errp in blkconf_geometry

[Andreas Färber]

Am 11.08.2014 09:45, schrieb Fam Zheng:
> This allows us to pass error information to caller.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 1/2] block: Pass errp in blkconf_geometry

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

On Mon, Aug 11, 2014 at 04:45:17PM +0800, Fam Zheng wrote:
> This allows us to pass error information to caller.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/block/block.c         | 18 +++++++++---------
>  hw/block/virtio-blk.c    |  7 +++----
>  hw/ide/qdev.c            | 11 ++++++++---
>  hw/scsi/scsi-disk.c      | 11 ++++++++---
>  include/hw/block/block.h |  6 ++++--
>  5 files changed, 32 insertions(+), 21 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 2/2] scsi-bus: Convert DeviceClass init to realize

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1901 bytes --]

On Mon, Aug 11, 2014 at 04:45:18PM +0800, Fam Zheng wrote:
> diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
> index d7b0f50..f6d9dc1 100644
> --- a/tests/qemu-iotests/051.out
> +++ b/tests/qemu-iotests/051.out
> @@ -122,7 +122,7 @@ QEMU_PROG: -drive if=virtio: Device 'virtio-blk-pci' could not be initialized
>  
>  Testing: -drive if=scsi
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) QEMU_PROG: -drive if=scsi: Device needs media, but drive is empty
> +(qemu) QEMU_PROG: Device needs media, but drive is empty
>  QEMU_PROG: Device initialization failed.
>  QEMU_PROG: Initialization of device lsi53c895a failed
>  

Have you checked where error_report()'s "location" was set to "-drive
if=scsi"?

Maybe something similar can be implemented.

> @@ -149,13 +149,11 @@ QEMU_PROG: -device ide-hd,drive=disk: Device 'ide-hd' could not be initialized
>  Testing: -drive if=none,id=disk -device lsi53c895a -device scsi-disk,drive=disk
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) QEMU_PROG: -device scsi-disk,drive=disk: Device needs media, but drive is empty
> -QEMU_PROG: -device scsi-disk,drive=disk: Device initialization failed.
>  QEMU_PROG: -device scsi-disk,drive=disk: Device 'scsi-disk' could not be initialized
>  
>  Testing: -drive if=none,id=disk -device lsi53c895a -device scsi-hd,drive=disk
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) QEMU_PROG: -device scsi-hd,drive=disk: Device needs media, but drive is empty
> -QEMU_PROG: -device scsi-hd,drive=disk: Device initialization failed.
>  QEMU_PROG: -device scsi-hd,drive=disk: Device 'scsi-hd' could not be initialized

Not a big loss, I found the generic "Device initialization failed"
followed by a specific error message pointless.  After this patch we
only get the specific error message.  I'm happy with that.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3 2/2] scsi-bus: Convert DeviceClass init to realize

[Fam Zheng]

On Mon, 08/11 15:32, Stefan Hajnoczi wrote:
> On Mon, Aug 11, 2014 at 04:45:18PM +0800, Fam Zheng wrote:
> > diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
> > index d7b0f50..f6d9dc1 100644
> > --- a/tests/qemu-iotests/051.out
> > +++ b/tests/qemu-iotests/051.out
> > @@ -122,7 +122,7 @@ QEMU_PROG: -drive if=virtio: Device 'virtio-blk-pci' could not be initialized
> >  
> >  Testing: -drive if=scsi
> >  QEMU X.Y.Z monitor - type 'help' for more information
> > -(qemu) QEMU_PROG: -drive if=scsi: Device needs media, but drive is empty
> > +(qemu) QEMU_PROG: Device needs media, but drive is empty
> >  QEMU_PROG: Device initialization failed.
> >  QEMU_PROG: Initialization of device lsi53c895a failed
> >  
> 
> Have you checked where error_report()'s "location" was set to "-drive
> if=scsi"?
> 
> Maybe something similar can be implemented.

Yes. In scsi_bus_legacy_handle_cmdline (callee of lsi, location is maintained until function
return. Since its callers only use "err" for failure detection, but not for
reporting (the specific information) to user, let's call error_report in
scsi_bus_legacy_handle_cmdline.

Thanks,
Fam

Re: [Qemu-devel] [PATCH v3 2/2] scsi-bus: Convert DeviceClass init to realize

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

On Tue, Aug 12, 2014 at 10:04:11AM +0800, Fam Zheng wrote:
> On Mon, 08/11 15:32, Stefan Hajnoczi wrote:
> > On Mon, Aug 11, 2014 at 04:45:18PM +0800, Fam Zheng wrote:
> > > diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
> > > index d7b0f50..f6d9dc1 100644
> > > --- a/tests/qemu-iotests/051.out
> > > +++ b/tests/qemu-iotests/051.out
> > > @@ -122,7 +122,7 @@ QEMU_PROG: -drive if=virtio: Device 'virtio-blk-pci' could not be initialized
> > >  
> > >  Testing: -drive if=scsi
> > >  QEMU X.Y.Z monitor - type 'help' for more information
> > > -(qemu) QEMU_PROG: -drive if=scsi: Device needs media, but drive is empty
> > > +(qemu) QEMU_PROG: Device needs media, but drive is empty
> > >  QEMU_PROG: Device initialization failed.
> > >  QEMU_PROG: Initialization of device lsi53c895a failed
> > >  
> > 
> > Have you checked where error_report()'s "location" was set to "-drive
> > if=scsi"?
> > 
> > Maybe something similar can be implemented.
> 
> Yes. In scsi_bus_legacy_handle_cmdline (callee of lsi, location is maintained until function
> return. Since its callers only use "err" for failure detection, but not for
> reporting (the specific information) to user, let's call error_report in
> scsi_bus_legacy_handle_cmdline.

Another option is:

  void error_prepend(Error **dst_errp, Error *local_err, const char *fmt, ...);

Examples:

  error_prepend(errp, NULL, "-drive if=scsi") -> NULL
  error_prepend(errp, "initialization failed", "-drive if=scsi") -> "-drive if=scsi: initialization failed"

So basically like error_propagate() except it also adds a prefix to the
error message.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]