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

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

[Fam Zheng]

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         | 64 +++++++++++++++++------------------
 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, 125 insertions(+), 114 deletions(-)

-- 
2.0.3

[Qemu-devel] [PATCH 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 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 uses
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 the callee's error_report is
changed to error_segs.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/lsi53c895a.c       |  2 ++
 hw/scsi/scsi-bus.c         | 64 ++++++++++++++++++-------------------
 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, 96 insertions(+), 96 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..85d1287 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,40 @@ 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) {
+    scsi_device_realize(dev, &local_err);
+    if (local_err) {
         dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
                                                          dev);
+        error_propagate(errp, local_err);
     }
 
     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 +1960,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 0/2] scsi: Change device init to realize

[Paolo Bonzini]

Il 05/08/2014 11:11, Fam Zheng ha scritto:
> 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.

I think the new message is an improvement.

Kevin/Stefan, are you taking both patches or can you alternatively ack
the first?

Paolo

> 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         | 64 +++++++++++++++++------------------
>  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, 125 insertions(+), 114 deletions(-)
> 

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

[Fam Zheng]

On Tue, 08/05 17:11, Fam Zheng wrote:
> Replace "init/destroy" with "realize/unrealize" in SCSIDeviceClass,
> which has errp as a parameter. So all the implementations now uses

s/uses/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 the callee's error_report is
> changed to error_segs.

s/error_segs/error_setg/

Fam

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

[Kevin Wolf]

Am 05.08.2014 um 11:11 hat Fam Zheng geschrieben:
> Replace "init/destroy" with "realize/unrealize" in SCSIDeviceClass,
> which has errp as a parameter. So all the implementations now uses
> 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 the callee's error_report is
> changed to error_segs.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/lsi53c895a.c       |  2 ++
>  hw/scsi/scsi-bus.c         | 64 ++++++++++++++++++-------------------
>  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, 96 insertions(+), 96 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;
>          }

Wouldn't qerror_report_err() be more useful? Or is already a QMP error
emitted in a different place in the callchain?

The same question is true for the added error_report() calls in patch 1.

> @@ -169,43 +168,40 @@ 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) {
> +    scsi_device_realize(dev, &local_err);
> +    if (local_err) {
>          dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
>                                                           dev);
> +        error_propagate(errp, local_err);
>      }

Maybe I'm misunderstanding something, but it looks to me as if the
handler was previously installed in case of success, and now it's only
installed on failure?

>  
>      if (bus->info->hotplug) {
>          bus->info->hotplug(bus, dev);
>      }
> -
> -err:
> -    return rc;
>  }

> 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

The old error message was certainly more useful. Not sure if there's a
good way to retain it, though.

Kevin

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

[Fam Zheng]

On Thu, 08/07 16:25, Kevin Wolf wrote:
> Am 05.08.2014 um 11:11 hat Fam Zheng geschrieben:
> > Replace "init/destroy" with "realize/unrealize" in SCSIDeviceClass,
> > which has errp as a parameter. So all the implementations now uses
> > 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 the callee's error_report is
> > changed to error_segs.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  hw/scsi/lsi53c895a.c       |  2 ++
> >  hw/scsi/scsi-bus.c         | 64 ++++++++++++++++++-------------------
> >  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, 96 insertions(+), 96 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;
> >          }
> 
> Wouldn't qerror_report_err() be more useful? Or is already a QMP error
> emitted in a different place in the callchain?

This code block is specifically for command line, see above "if
(!d->hotplugged)" condition check and "scsi_bus_legacy_handle_cmdline(&s->bus,
&err);" call. So I think error_report is good enough.

> 
> The same question is true for the added error_report() calls in patch 1.

Two error_report() added in patch 1: the first is consistent with the other
error_report() in the context function; the second is replaced by error_setg in
this patch when errp is added.

> 
> > @@ -169,43 +168,40 @@ 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) {
> > +    scsi_device_realize(dev, &local_err);
> > +    if (local_err) {
> >          dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
> >                                                           dev);
> > +        error_propagate(errp, local_err);
> >      }
> 
> Maybe I'm misunderstanding something, but it looks to me as if the
> handler was previously installed in case of success, and now it's only
> installed on failure?

Good catch! Will fix.

Thanks,
Fam