* [Qemu-devel] [PATCH v2 1/6] hw/ide: Convert DeviceClass init to realize
2017-08-04 10:26 [Qemu-devel] [PATCH v2 0/6] Convert to realize and improve error handling Mao Zhongyi
@ 2017-08-04 10:26 ` Mao Zhongyi
2017-09-15 21:35 ` John Snow
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 2/6] hw/block/fdc: Convert " Mao Zhongyi
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Mao Zhongyi @ 2017-08-04 10:26 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: John Snow, Markus Armbruster
Replace init with realize in IDEDeviceClass, which has errp
as a parameter. So all the implementations now use error_setg
instead of error_report for reporting error.
Cc: John Snow <jsnow@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/ide/core.c | 7 ++--
hw/ide/qdev.c | 86 +++++++++++++++++++++++------------------------
include/hw/ide/internal.h | 5 +--
3 files changed, 49 insertions(+), 49 deletions(-)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0b48b64..7c86fc7 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -34,6 +34,7 @@
#include "hw/block/block.h"
#include "sysemu/block-backend.h"
#include "qemu/cutils.h"
+#include "qemu/error-report.h"
#include "hw/ide/internal.h"
@@ -2398,7 +2399,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
const char *version, const char *serial, const char *model,
uint64_t wwn,
uint32_t cylinders, uint32_t heads, uint32_t secs,
- int chs_trans)
+ int chs_trans, Error **errp)
{
uint64_t nb_sectors;
@@ -2423,11 +2424,11 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
blk_set_guest_block_size(blk, 2048);
} else {
if (!blk_is_inserted(s->blk)) {
- error_report("Device needs media, but drive is empty");
+ error_setg(errp, "Device needs media, but drive is empty");
return -1;
}
if (blk_is_read_only(blk)) {
- error_report("Can't use a read-only drive");
+ error_setg(errp, "Can't use a read-only drive");
return -1;
}
blk_set_dev_ops(blk, &ide_hd_block_ops, s);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index cc2f5bd..d60ac25 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -80,7 +80,7 @@ static char *idebus_get_fw_dev_path(DeviceState *dev)
return g_strdup(path);
}
-static int ide_qdev_init(DeviceState *qdev)
+static void ide_qdev_realize(DeviceState *qdev, Error **errp)
{
IDEDevice *dev = IDE_DEVICE(qdev);
IDEDeviceClass *dc = IDE_DEVICE_GET_CLASS(dev);
@@ -91,34 +91,31 @@ static int ide_qdev_init(DeviceState *qdev)
}
if (dev->unit >= bus->max_units) {
- error_report("Can't create IDE unit %d, bus supports only %d units",
+ error_setg(errp, "Can't create IDE unit %d, bus supports only %d units",
dev->unit, bus->max_units);
- goto err;
+ return;
}
switch (dev->unit) {
case 0:
if (bus->master) {
- error_report("IDE unit %d is in use", dev->unit);
- goto err;
+ error_setg(errp, "IDE unit %d is in use", dev->unit);
+ return;
}
bus->master = dev;
break;
case 1:
if (bus->slave) {
- error_report("IDE unit %d is in use", dev->unit);
- goto err;
+ error_setg(errp, "IDE unit %d is in use", dev->unit);
+ return;
}
bus->slave = dev;
break;
default:
- error_report("Invalid IDE unit %d", dev->unit);
- goto err;
+ error_setg(errp, "Invalid IDE unit %d", dev->unit);
+ return;
}
- return dc->init(dev);
-
-err:
- return -1;
+ dc->realize(dev, errp);
}
IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
@@ -159,7 +156,7 @@ typedef struct IDEDrive {
IDEDevice dev;
} IDEDrive;
-static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
+static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
{
IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
IDEState *s = bus->ifs + dev->unit;
@@ -168,8 +165,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
if (!dev->conf.blk) {
if (kind != IDE_CD) {
- error_report("No drive specified");
- return -1;
+ error_setg(errp, "No drive specified");
+ return;
} else {
/* Anonymous BlockBackend for an empty drive */
dev->conf.blk = blk_new(0, BLK_PERM_ALL);
@@ -182,36 +179,36 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
dev->conf.discard_granularity = 512;
} else if (dev->conf.discard_granularity &&
dev->conf.discard_granularity != 512) {
- error_report("discard_granularity must be 512 for ide");
- return -1;
+ error_setg(errp, "discard_granularity must be 512 for ide");
+ return;
}
blkconf_blocksizes(&dev->conf);
if (dev->conf.logical_block_size != 512) {
- error_report("logical_block_size must be 512 for IDE");
- return -1;
+ error_setg(errp, "logical_block_size must be 512 for IDE");
+ return;
}
blkconf_serial(&dev->conf, &dev->serial);
if (kind != IDE_CD) {
blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255, &err);
if (err) {
- error_report_err(err);
- return -1;
+ error_propagate(errp, err);
+ return;
}
}
blkconf_apply_backend_options(&dev->conf, kind == IDE_CD, kind != IDE_CD,
&err);
if (err) {
- error_report_err(err);
- return -1;
+ error_propagate(errp, err);
+ return;
}
if (ide_init_drive(s, dev->conf.blk, kind,
dev->version, dev->serial, dev->model, dev->wwn,
dev->conf.cyls, dev->conf.heads, dev->conf.secs,
- dev->chs_trans) < 0) {
- return -1;
+ dev->chs_trans, errp) < 0) {
+ return;
}
if (!dev->version) {
@@ -223,8 +220,6 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
add_boot_device_path(dev->conf.bootindex, &dev->qdev,
dev->unit ? "/disk@1" : "/disk@0");
-
- return 0;
}
static void ide_dev_get_bootindex(Object *obj, Visitor *v, const char *name,
@@ -270,17 +265,17 @@ static void ide_dev_instance_init(Object *obj)
object_property_set_int(obj, -1, "bootindex", NULL);
}
-static int ide_hd_initfn(IDEDevice *dev)
+static void ide_hd_realize(IDEDevice *dev, Error **errp)
{
- return ide_dev_initfn(dev, IDE_HD);
+ ide_dev_initfn(dev, IDE_HD, errp);
}
-static int ide_cd_initfn(IDEDevice *dev)
+static void ide_cd_realize(IDEDevice *dev, Error **errp)
{
- return ide_dev_initfn(dev, IDE_CD);
+ ide_dev_initfn(dev, IDE_CD, errp);
}
-static int ide_drive_initfn(IDEDevice *dev)
+static void ide_drive_realize(IDEDevice *dev, Error **errp)
{
DriveInfo *dinfo = NULL;
@@ -288,7 +283,7 @@ static int ide_drive_initfn(IDEDevice *dev)
dinfo = blk_legacy_dinfo(dev->conf.blk);
}
- return ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD);
+ ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD, errp);
}
#define DEFINE_IDE_DEV_PROPERTIES() \
@@ -311,10 +306,11 @@ static void ide_hd_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
- k->init = ide_hd_initfn;
+
+ k->realize = ide_hd_realize;
dc->fw_name = "drive";
- dc->desc = "virtual IDE disk";
- dc->props = ide_hd_properties;
+ dc->desc = "virtual IDE disk";
+ dc->props = ide_hd_properties;
}
static const TypeInfo ide_hd_info = {
@@ -333,10 +329,11 @@ static void ide_cd_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
- k->init = ide_cd_initfn;
+
+ k->realize = ide_cd_realize;
dc->fw_name = "drive";
- dc->desc = "virtual IDE CD-ROM";
- dc->props = ide_cd_properties;
+ dc->desc = "virtual IDE CD-ROM";
+ dc->props = ide_cd_properties;
}
static const TypeInfo ide_cd_info = {
@@ -355,10 +352,11 @@ static void ide_drive_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
- k->init = ide_drive_initfn;
+
+ k->realize = ide_drive_realize;
dc->fw_name = "drive";
- dc->desc = "virtual IDE disk or CD-ROM (legacy)";
- dc->props = ide_drive_properties;
+ dc->desc = "virtual IDE disk or CD-ROM (legacy)";
+ dc->props = ide_drive_properties;
}
static const TypeInfo ide_drive_info = {
@@ -371,7 +369,7 @@ static const TypeInfo ide_drive_info = {
static void ide_device_class_init(ObjectClass *klass, void *data)
{
DeviceClass *k = DEVICE_CLASS(klass);
- k->init = ide_qdev_init;
+ k->realize = ide_qdev_realize;
set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
k->bus_type = TYPE_IDE_BUS;
k->props = ide_props;
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 482a951..5ebb4c9 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -12,6 +12,7 @@
#include "sysemu/sysemu.h"
#include "hw/block/block.h"
#include "block/scsi.h"
+#include "qapi/error.h"
/* debug IDE devices */
//#define DEBUG_IDE
@@ -495,7 +496,7 @@ struct IDEBus {
typedef struct IDEDeviceClass {
DeviceClass parent_class;
- int (*init)(IDEDevice *dev);
+ void (*realize)(IDEDevice *dev, Error **errp);
} IDEDeviceClass;
struct IDEDevice {
@@ -605,7 +606,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
const char *version, const char *serial, const char *model,
uint64_t wwn,
uint32_t cylinders, uint32_t heads, uint32_t secs,
- int chs_trans);
+ int chs_trans, Error **errp);
void ide_init2(IDEBus *bus, qemu_irq irq);
void ide_exit(IDEState *s);
void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/6] hw/ide: Convert DeviceClass init to realize
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 1/6] hw/ide: Convert DeviceClass init to realize Mao Zhongyi
@ 2017-09-15 21:35 ` John Snow
2017-09-15 21:42 ` Eric Blake
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: John Snow @ 2017-09-15 21:35 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel, qemu-block; +Cc: Markus Armbruster
On 08/04/2017 06:26 AM, Mao Zhongyi wrote:
> Replace init with realize in IDEDeviceClass, which has errp
> as a parameter. So all the implementations now use error_setg
> instead of error_report for reporting error.
>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> hw/ide/core.c | 7 ++--
> hw/ide/qdev.c | 86 +++++++++++++++++++++++------------------------
> include/hw/ide/internal.h | 5 +--
> 3 files changed, 49 insertions(+), 49 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 0b48b64..7c86fc7 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -34,6 +34,7 @@
> #include "hw/block/block.h"
> #include "sysemu/block-backend.h"
> #include "qemu/cutils.h"
> +#include "qemu/error-report.h"
>
> #include "hw/ide/internal.h"
>
> @@ -2398,7 +2399,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
> const char *version, const char *serial, const char *model,
> uint64_t wwn,
> uint32_t cylinders, uint32_t heads, uint32_t secs,
> - int chs_trans)
> + int chs_trans, Error **errp)
this function now requires an additional invariant, which is that we
must return -1 AND set errp. Probably wisest to just get rid of the
return code so that we don't accidentally goof this up in the future.
I think Markus has had some guidance on this in the past, but admittedly
I can't remember his preference.
> {
> uint64_t nb_sectors;
>
> @@ -2423,11 +2424,11 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
> blk_set_guest_block_size(blk, 2048);
> } else {
> if (!blk_is_inserted(s->blk)) {
> - error_report("Device needs media, but drive is empty");
> + error_setg(errp, "Device needs media, but drive is empty");
> return -1;
> }
> if (blk_is_read_only(blk)) {
> - error_report("Can't use a read-only drive");
> + error_setg(errp, "Can't use a read-only drive");
> return -1;
> }
> blk_set_dev_ops(blk, &ide_hd_block_ops, s);
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index cc2f5bd..d60ac25 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -80,7 +80,7 @@ static char *idebus_get_fw_dev_path(DeviceState *dev)
> return g_strdup(path);
> }
>
> -static int ide_qdev_init(DeviceState *qdev)
> +static void ide_qdev_realize(DeviceState *qdev, Error **errp)
> {
> IDEDevice *dev = IDE_DEVICE(qdev);
> IDEDeviceClass *dc = IDE_DEVICE_GET_CLASS(dev);
> @@ -91,34 +91,31 @@ static int ide_qdev_init(DeviceState *qdev)
> }
>
> if (dev->unit >= bus->max_units) {
> - error_report("Can't create IDE unit %d, bus supports only %d units",
> + error_setg(errp, "Can't create IDE unit %d, bus supports only %d units",
> dev->unit, bus->max_units);
> - goto err;
> + return;
> }
>
> switch (dev->unit) {
> case 0:
> if (bus->master) {
> - error_report("IDE unit %d is in use", dev->unit);
> - goto err;
> + error_setg(errp, "IDE unit %d is in use", dev->unit);
> + return;
> }
> bus->master = dev;
> break;
> case 1:
> if (bus->slave) {
> - error_report("IDE unit %d is in use", dev->unit);
> - goto err;
> + error_setg(errp, "IDE unit %d is in use", dev->unit);
> + return;
> }
> bus->slave = dev;
> break;
> default:
> - error_report("Invalid IDE unit %d", dev->unit);
> - goto err;
> + error_setg(errp, "Invalid IDE unit %d", dev->unit);
> + return;
> }
> - return dc->init(dev);
> -
> -err:
> - return -1;
> + dc->realize(dev, errp);
> }
>
> IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
> @@ -159,7 +156,7 @@ typedef struct IDEDrive {
> IDEDevice dev;
> } IDEDrive;
>
> -static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
> +static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
> {
> IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
> IDEState *s = bus->ifs + dev->unit;
> @@ -168,8 +165,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>
> if (!dev->conf.blk) {
> if (kind != IDE_CD) {
> - error_report("No drive specified");
> - return -1;
> + error_setg(errp, "No drive specified");
> + return;
> } else {
> /* Anonymous BlockBackend for an empty drive */
> dev->conf.blk = blk_new(0, BLK_PERM_ALL);
> @@ -182,36 +179,36 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
> dev->conf.discard_granularity = 512;
> } else if (dev->conf.discard_granularity &&
> dev->conf.discard_granularity != 512) {
> - error_report("discard_granularity must be 512 for ide");
> - return -1;
> + error_setg(errp, "discard_granularity must be 512 for ide");
> + return;
> }
>
> blkconf_blocksizes(&dev->conf);
> if (dev->conf.logical_block_size != 512) {
> - error_report("logical_block_size must be 512 for IDE");
> - return -1;
> + error_setg(errp, "logical_block_size must be 512 for IDE");
> + return;
> }
>
> blkconf_serial(&dev->conf, &dev->serial);
> if (kind != IDE_CD) {
> blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255, &err);
> if (err) {
> - error_report_err(err);
> - return -1;
> + error_propagate(errp, err);
> + return;
> }
> }
> blkconf_apply_backend_options(&dev->conf, kind == IDE_CD, kind != IDE_CD,
> &err);
> if (err) {
> - error_report_err(err);
> - return -1;
> + error_propagate(errp, err);
> + return;
> }
>
> if (ide_init_drive(s, dev->conf.blk, kind,
> dev->version, dev->serial, dev->model, dev->wwn,
> dev->conf.cyls, dev->conf.heads, dev->conf.secs,
> - dev->chs_trans) < 0) {
> - return -1;
> + dev->chs_trans, errp) < 0) {
> + return;
> }
>
> if (!dev->version) {
> @@ -223,8 +220,6 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>
> add_boot_device_path(dev->conf.bootindex, &dev->qdev,
> dev->unit ? "/disk@1" : "/disk@0");
> -
> - return 0;
> }
>
> static void ide_dev_get_bootindex(Object *obj, Visitor *v, const char *name,
> @@ -270,17 +265,17 @@ static void ide_dev_instance_init(Object *obj)
> object_property_set_int(obj, -1, "bootindex", NULL);
> }
>
> -static int ide_hd_initfn(IDEDevice *dev)
> +static void ide_hd_realize(IDEDevice *dev, Error **errp)
why rename these but not ide_dev_initfn ? I guess because it's not
directly referenced as being a realize function.
> {
> - return ide_dev_initfn(dev, IDE_HD);
> + ide_dev_initfn(dev, IDE_HD, errp);
> }
>
> -static int ide_cd_initfn(IDEDevice *dev)
> +static void ide_cd_realize(IDEDevice *dev, Error **errp)
> {
> - return ide_dev_initfn(dev, IDE_CD);
> + ide_dev_initfn(dev, IDE_CD, errp);
> }
>
> -static int ide_drive_initfn(IDEDevice *dev)
> +static void ide_drive_realize(IDEDevice *dev, Error **errp)
> {
> DriveInfo *dinfo = NULL;
>
> @@ -288,7 +283,7 @@ static int ide_drive_initfn(IDEDevice *dev)
> dinfo = blk_legacy_dinfo(dev->conf.blk);
> }
>
> - return ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD);
> + ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD, errp);
> }
>
> #define DEFINE_IDE_DEV_PROPERTIES() \
> @@ -311,10 +306,11 @@ static void ide_hd_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
> - k->init = ide_hd_initfn;
> +
> + k->realize = ide_hd_realize;
> dc->fw_name = "drive";
> - dc->desc = "virtual IDE disk";
> - dc->props = ide_hd_properties;
> + dc->desc = "virtual IDE disk";
> + dc->props = ide_hd_properties;
> }
>
> static const TypeInfo ide_hd_info = {
> @@ -333,10 +329,11 @@ static void ide_cd_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
> - k->init = ide_cd_initfn;
> +
> + k->realize = ide_cd_realize;
> dc->fw_name = "drive";
> - dc->desc = "virtual IDE CD-ROM";
> - dc->props = ide_cd_properties;
> + dc->desc = "virtual IDE CD-ROM";
> + dc->props = ide_cd_properties;
> }
>
> static const TypeInfo ide_cd_info = {
> @@ -355,10 +352,11 @@ static void ide_drive_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
> - k->init = ide_drive_initfn;
> +
> + k->realize = ide_drive_realize;
> dc->fw_name = "drive";
> - dc->desc = "virtual IDE disk or CD-ROM (legacy)";
> - dc->props = ide_drive_properties;
> + dc->desc = "virtual IDE disk or CD-ROM (legacy)";
> + dc->props = ide_drive_properties;
> }
>
> static const TypeInfo ide_drive_info = {
> @@ -371,7 +369,7 @@ static const TypeInfo ide_drive_info = {
> static void ide_device_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *k = DEVICE_CLASS(klass);
> - k->init = ide_qdev_init;
> + k->realize = ide_qdev_realize;
> set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
> k->bus_type = TYPE_IDE_BUS;
> k->props = ide_props;
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index 482a951..5ebb4c9 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -12,6 +12,7 @@
> #include "sysemu/sysemu.h"
> #include "hw/block/block.h"
> #include "block/scsi.h"
> +#include "qapi/error.h"
>
> /* debug IDE devices */
> //#define DEBUG_IDE
> @@ -495,7 +496,7 @@ struct IDEBus {
>
> typedef struct IDEDeviceClass {
> DeviceClass parent_class;
> - int (*init)(IDEDevice *dev);
> + void (*realize)(IDEDevice *dev, Error **errp);
> } IDEDeviceClass;
>
> struct IDEDevice {
> @@ -605,7 +606,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
> const char *version, const char *serial, const char *model,
> uint64_t wwn,
> uint32_t cylinders, uint32_t heads, uint32_t secs,
> - int chs_trans);
> + int chs_trans, Error **errp);
> void ide_init2(IDEBus *bus, qemu_irq irq);
> void ide_exit(IDEState *s);
> void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>
Only matters of style; seems mechanically OK.
Reviewed-by: John Snow <jsnow@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/6] hw/ide: Convert DeviceClass init to realize
2017-09-15 21:35 ` John Snow
@ 2017-09-15 21:42 ` Eric Blake
2017-09-15 21:46 ` John Snow
2017-09-15 22:03 ` [Qemu-devel] [Qemu-block] " John Snow
2017-09-18 1:35 ` [Qemu-devel] " Mao Zhongyi
2 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2017-09-15 21:42 UTC (permalink / raw)
To: John Snow, Mao Zhongyi, qemu-devel, qemu-block; +Cc: Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]
On 09/15/2017 04:35 PM, John Snow wrote:
>
>
> On 08/04/2017 06:26 AM, Mao Zhongyi wrote:
>> Replace init with realize in IDEDeviceClass, which has errp
>> as a parameter. So all the implementations now use error_setg
>> instead of error_report for reporting error.
>>
>> @@ -2398,7 +2399,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>> const char *version, const char *serial, const char *model,
>> uint64_t wwn,
>> uint32_t cylinders, uint32_t heads, uint32_t secs,
>> - int chs_trans)
>> + int chs_trans, Error **errp)
>
> this function now requires an additional invariant, which is that we
> must return -1 AND set errp. Probably wisest to just get rid of the
> return code so that we don't accidentally goof this up in the future.
>
> I think Markus has had some guidance on this in the past, but admittedly
> I can't remember his preference.
Returning void requires callers to use boilerplate:
bar(..., Error **errp)
{
Error *err = NULL;
foo(..., &err);
if (err) {
error_propagate(errp, err);
handle error ...;
return;
}
}
whereas keeping the invariant that a -1 return is synonymous with err
being set is simpler:
bar(..., Error **errp)
{
if (foo(..., errp) < 0) {
handle error ...;
return;
}
}
So these days, the preference is to KEEP the redundancy rather than
eliminate it, due to ease-of-use concerns.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/6] hw/ide: Convert DeviceClass init to realize
2017-09-15 21:42 ` Eric Blake
@ 2017-09-15 21:46 ` John Snow
0 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2017-09-15 21:46 UTC (permalink / raw)
To: Eric Blake, Mao Zhongyi, qemu-devel, qemu-block; +Cc: Markus Armbruster
On 09/15/2017 05:42 PM, Eric Blake wrote:
> On 09/15/2017 04:35 PM, John Snow wrote:
>>
>>
>> On 08/04/2017 06:26 AM, Mao Zhongyi wrote:
>>> Replace init with realize in IDEDeviceClass, which has errp
>>> as a parameter. So all the implementations now use error_setg
>>> instead of error_report for reporting error.
>>>
>
>>> @@ -2398,7 +2399,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>>> const char *version, const char *serial, const char *model,
>>> uint64_t wwn,
>>> uint32_t cylinders, uint32_t heads, uint32_t secs,
>>> - int chs_trans)
>>> + int chs_trans, Error **errp)
>>
>> this function now requires an additional invariant, which is that we
>> must return -1 AND set errp. Probably wisest to just get rid of the
>> return code so that we don't accidentally goof this up in the future.
>>
>> I think Markus has had some guidance on this in the past, but admittedly
>> I can't remember his preference.
>
[...]
> So these days, the preference is to KEEP the redundancy rather than
> eliminate it, due to ease-of-use concerns.
>
OK, that's fine. I only want consistency with whatever the
paradigm-du-jour is for error handling. Thanks for the quick rebuttal.
--js
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/6] hw/ide: Convert DeviceClass init to realize
2017-09-15 21:35 ` John Snow
2017-09-15 21:42 ` Eric Blake
@ 2017-09-15 22:03 ` John Snow
2017-09-18 1:37 ` Mao Zhongyi
2017-09-18 1:35 ` [Qemu-devel] " Mao Zhongyi
2 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2017-09-15 22:03 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel, qemu-block; +Cc: Markus Armbruster
On 09/15/2017 05:35 PM, John Snow wrote:
>
>
> On 08/04/2017 06:26 AM, Mao Zhongyi wrote:
>> Replace init with realize in IDEDeviceClass, which has errp
>> as a parameter. So all the implementations now use error_setg
>> instead of error_report for reporting error.
>>
>> Cc: John Snow <jsnow@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>>
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> hw/ide/core.c | 7 ++--
>> hw/ide/qdev.c | 86 +++++++++++++++++++++++------------------------
>> include/hw/ide/internal.h | 5 +--
>> 3 files changed, 49 insertions(+), 49 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 0b48b64..7c86fc7 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -34,6 +34,7 @@
>> #include "hw/block/block.h"
>> #include "sysemu/block-backend.h"
>> #include "qemu/cutils.h"
>> +#include "qemu/error-report.h"
>>
>> #include "hw/ide/internal.h"
>>
>> @@ -2398,7 +2399,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>> const char *version, const char *serial, const char *model,
>> uint64_t wwn,
>> uint32_t cylinders, uint32_t heads, uint32_t secs,
>> - int chs_trans)
>> + int chs_trans, Error **errp)
>
> this function now requires an additional invariant, which is that we
> must return -1 AND set errp. Probably wisest to just get rid of the
> return code so that we don't accidentally goof this up in the future.
>
> I think Markus has had some guidance on this in the past, but admittedly
> I can't remember his preference.
>
>> {
>> uint64_t nb_sectors;
>>
>> @@ -2423,11 +2424,11 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>> blk_set_guest_block_size(blk, 2048);
>> } else {
>> if (!blk_is_inserted(s->blk)) {
>> - error_report("Device needs media, but drive is empty");
>> + error_setg(errp, "Device needs media, but drive is empty");
>> return -1;
>> }
>> if (blk_is_read_only(blk)) {
>> - error_report("Can't use a read-only drive");
>> + error_setg(errp, "Can't use a read-only drive");
>> return -1;
>> }
>> blk_set_dev_ops(blk, &ide_hd_block_ops, s);
>> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
>> index cc2f5bd..d60ac25 100644
>> --- a/hw/ide/qdev.c
>> +++ b/hw/ide/qdev.c
>> @@ -80,7 +80,7 @@ static char *idebus_get_fw_dev_path(DeviceState *dev)
>> return g_strdup(path);
>> }
>>
>> -static int ide_qdev_init(DeviceState *qdev)
>> +static void ide_qdev_realize(DeviceState *qdev, Error **errp)
>> {
>> IDEDevice *dev = IDE_DEVICE(qdev);
>> IDEDeviceClass *dc = IDE_DEVICE_GET_CLASS(dev);
>> @@ -91,34 +91,31 @@ static int ide_qdev_init(DeviceState *qdev)
>> }
>>
>> if (dev->unit >= bus->max_units) {
>> - error_report("Can't create IDE unit %d, bus supports only %d units",
>> + error_setg(errp, "Can't create IDE unit %d, bus supports only %d units",
>> dev->unit, bus->max_units);
>> - goto err;
>> + return;
>> }
>>
>> switch (dev->unit) {
>> case 0:
>> if (bus->master) {
>> - error_report("IDE unit %d is in use", dev->unit);
>> - goto err;
>> + error_setg(errp, "IDE unit %d is in use", dev->unit);
>> + return;
>> }
>> bus->master = dev;
>> break;
>> case 1:
>> if (bus->slave) {
>> - error_report("IDE unit %d is in use", dev->unit);
>> - goto err;
>> + error_setg(errp, "IDE unit %d is in use", dev->unit);
>> + return;
>> }
>> bus->slave = dev;
>> break;
>> default:
>> - error_report("Invalid IDE unit %d", dev->unit);
>> - goto err;
>> + error_setg(errp, "Invalid IDE unit %d", dev->unit);
>> + return;
>> }
>> - return dc->init(dev);
>> -
>> -err:
>> - return -1;
>> + dc->realize(dev, errp);
>> }
>>
>> IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
>> @@ -159,7 +156,7 @@ typedef struct IDEDrive {
>> IDEDevice dev;
>> } IDEDrive;
>>
>> -static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>> +static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
>> {
>> IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
>> IDEState *s = bus->ifs + dev->unit;
>> @@ -168,8 +165,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>
>> if (!dev->conf.blk) {
>> if (kind != IDE_CD) {
>> - error_report("No drive specified");
>> - return -1;
>> + error_setg(errp, "No drive specified");
>> + return;
>> } else {
>> /* Anonymous BlockBackend for an empty drive */
>> dev->conf.blk = blk_new(0, BLK_PERM_ALL);
>> @@ -182,36 +179,36 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>> dev->conf.discard_granularity = 512;
>> } else if (dev->conf.discard_granularity &&
>> dev->conf.discard_granularity != 512) {
>> - error_report("discard_granularity must be 512 for ide");
>> - return -1;
>> + error_setg(errp, "discard_granularity must be 512 for ide");
>> + return;
>> }
>>
>> blkconf_blocksizes(&dev->conf);
>> if (dev->conf.logical_block_size != 512) {
>> - error_report("logical_block_size must be 512 for IDE");
>> - return -1;
>> + error_setg(errp, "logical_block_size must be 512 for IDE");
>> + return;
>> }
>>
>> blkconf_serial(&dev->conf, &dev->serial);
>> if (kind != IDE_CD) {
>> blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255, &err);
>> if (err) {
>> - error_report_err(err);
>> - return -1;
>> + error_propagate(errp, err);
>> + return;
>> }
>> }
>> blkconf_apply_backend_options(&dev->conf, kind == IDE_CD, kind != IDE_CD,
>> &err);
>> if (err) {
>> - error_report_err(err);
>> - return -1;
>> + error_propagate(errp, err);
>> + return;
>> }
>>
>> if (ide_init_drive(s, dev->conf.blk, kind,
>> dev->version, dev->serial, dev->model, dev->wwn,
>> dev->conf.cyls, dev->conf.heads, dev->conf.secs,
>> - dev->chs_trans) < 0) {
>> - return -1;
>> + dev->chs_trans, errp) < 0) {
>> + return;
>> }
>>
>> if (!dev->version) {
>> @@ -223,8 +220,6 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>
>> add_boot_device_path(dev->conf.bootindex, &dev->qdev,
>> dev->unit ? "/disk@1" : "/disk@0");
>> -
>> - return 0;
>> }
>>
>> static void ide_dev_get_bootindex(Object *obj, Visitor *v, const char *name,
>> @@ -270,17 +265,17 @@ static void ide_dev_instance_init(Object *obj)
>> object_property_set_int(obj, -1, "bootindex", NULL);
>> }
>>
>> -static int ide_hd_initfn(IDEDevice *dev)
>> +static void ide_hd_realize(IDEDevice *dev, Error **errp)
>
> why rename these but not ide_dev_initfn ? I guess because it's not
> directly referenced as being a realize function.
>
>> {
>> - return ide_dev_initfn(dev, IDE_HD);
>> + ide_dev_initfn(dev, IDE_HD, errp);
>> }
>>
>> -static int ide_cd_initfn(IDEDevice *dev)
>> +static void ide_cd_realize(IDEDevice *dev, Error **errp)
>> {
>> - return ide_dev_initfn(dev, IDE_CD);
>> + ide_dev_initfn(dev, IDE_CD, errp);
>> }
>>
>> -static int ide_drive_initfn(IDEDevice *dev)
>> +static void ide_drive_realize(IDEDevice *dev, Error **errp)
>> {
>> DriveInfo *dinfo = NULL;
>>
>> @@ -288,7 +283,7 @@ static int ide_drive_initfn(IDEDevice *dev)
>> dinfo = blk_legacy_dinfo(dev->conf.blk);
>> }
>>
>> - return ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD);
>> + ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD, errp);
>> }
>>
>> #define DEFINE_IDE_DEV_PROPERTIES() \
>> @@ -311,10 +306,11 @@ static void ide_hd_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
>> - k->init = ide_hd_initfn;
>> +
>> + k->realize = ide_hd_realize;
>> dc->fw_name = "drive";
>> - dc->desc = "virtual IDE disk";
>> - dc->props = ide_hd_properties;
>> + dc->desc = "virtual IDE disk";
>> + dc->props = ide_hd_properties;
>> }
>>
>> static const TypeInfo ide_hd_info = {
>> @@ -333,10 +329,11 @@ static void ide_cd_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
>> - k->init = ide_cd_initfn;
>> +
>> + k->realize = ide_cd_realize;
>> dc->fw_name = "drive";
>> - dc->desc = "virtual IDE CD-ROM";
>> - dc->props = ide_cd_properties;
>> + dc->desc = "virtual IDE CD-ROM";
>> + dc->props = ide_cd_properties;
>> }
>>
>> static const TypeInfo ide_cd_info = {
>> @@ -355,10 +352,11 @@ static void ide_drive_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
>> - k->init = ide_drive_initfn;
>> +
>> + k->realize = ide_drive_realize;
>> dc->fw_name = "drive";
>> - dc->desc = "virtual IDE disk or CD-ROM (legacy)";
>> - dc->props = ide_drive_properties;
>> + dc->desc = "virtual IDE disk or CD-ROM (legacy)";
>> + dc->props = ide_drive_properties;
>> }
>>
>> static const TypeInfo ide_drive_info = {
>> @@ -371,7 +369,7 @@ static const TypeInfo ide_drive_info = {
>> static void ide_device_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *k = DEVICE_CLASS(klass);
>> - k->init = ide_qdev_init;
>> + k->realize = ide_qdev_realize;
>> set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
>> k->bus_type = TYPE_IDE_BUS;
>> k->props = ide_props;
>> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
>> index 482a951..5ebb4c9 100644
>> --- a/include/hw/ide/internal.h
>> +++ b/include/hw/ide/internal.h
>> @@ -12,6 +12,7 @@
>> #include "sysemu/sysemu.h"
>> #include "hw/block/block.h"
>> #include "block/scsi.h"
>> +#include "qapi/error.h"
>>
>> /* debug IDE devices */
>> //#define DEBUG_IDE
>> @@ -495,7 +496,7 @@ struct IDEBus {
>>
>> typedef struct IDEDeviceClass {
>> DeviceClass parent_class;
>> - int (*init)(IDEDevice *dev);
>> + void (*realize)(IDEDevice *dev, Error **errp);
>> } IDEDeviceClass;
>>
>> struct IDEDevice {
>> @@ -605,7 +606,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>> const char *version, const char *serial, const char *model,
>> uint64_t wwn,
>> uint32_t cylinders, uint32_t heads, uint32_t secs,
>> - int chs_trans);
>> + int chs_trans, Error **errp);
>> void ide_init2(IDEBus *bus, qemu_irq irq);
>> void ide_exit(IDEState *s);
>> void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>>
>
> Only matters of style; seems mechanically OK.
>
> Reviewed-by: John Snow <jsnow@redhat.com>
>
Whoops, rescind that; this causes some test failures in test 051. You
need to update the test output in 051 to match the new failure modes.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/6] hw/ide: Convert DeviceClass init to realize
2017-09-15 22:03 ` [Qemu-devel] [Qemu-block] " John Snow
@ 2017-09-18 1:37 ` Mao Zhongyi
0 siblings, 0 replies; 17+ messages in thread
From: Mao Zhongyi @ 2017-09-18 1:37 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block; +Cc: Markus Armbruster
On 09/16/2017 06:03 AM, John Snow wrote:
>
>
> On 09/15/2017 05:35 PM, John Snow wrote:
>>
>>
>> On 08/04/2017 06:26 AM, Mao Zhongyi wrote:
>>> Replace init with realize in IDEDeviceClass, which has errp
>>> as a parameter. So all the implementations now use error_setg
>>> instead of error_report for reporting error.
>>>
>>> Cc: John Snow <jsnow@redhat.com>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>>
>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>> ---
>>> hw/ide/core.c | 7 ++--
>>> hw/ide/qdev.c | 86 +++++++++++++++++++++++------------------------
>>> include/hw/ide/internal.h | 5 +--
>>> 3 files changed, 49 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 0b48b64..7c86fc7 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -34,6 +34,7 @@
>>> #include "hw/block/block.h"
>>> #include "sysemu/block-backend.h"
>>> #include "qemu/cutils.h"
>>> +#include "qemu/error-report.h"
>>>
>>> #include "hw/ide/internal.h"
>>>
>>> @@ -2398,7 +2399,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>>> const char *version, const char *serial, const char *model,
>>> uint64_t wwn,
>>> uint32_t cylinders, uint32_t heads, uint32_t secs,
>>> - int chs_trans)
>>> + int chs_trans, Error **errp)
>>
>> this function now requires an additional invariant, which is that we
>> must return -1 AND set errp. Probably wisest to just get rid of the
>> return code so that we don't accidentally goof this up in the future.
>>
>> I think Markus has had some guidance on this in the past, but admittedly
>> I can't remember his preference.
>>
>>> {
>>> uint64_t nb_sectors;
>>>
>>> @@ -2423,11 +2424,11 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>>> blk_set_guest_block_size(blk, 2048);
>>> } else {
>>> if (!blk_is_inserted(s->blk)) {
>>> - error_report("Device needs media, but drive is empty");
>>> + error_setg(errp, "Device needs media, but drive is empty");
>>> return -1;
>>> }
>>> if (blk_is_read_only(blk)) {
>>> - error_report("Can't use a read-only drive");
>>> + error_setg(errp, "Can't use a read-only drive");
>>> return -1;
>>> }
>>> blk_set_dev_ops(blk, &ide_hd_block_ops, s);
>>> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
>>> index cc2f5bd..d60ac25 100644
>>> --- a/hw/ide/qdev.c
>>> +++ b/hw/ide/qdev.c
>>> @@ -80,7 +80,7 @@ static char *idebus_get_fw_dev_path(DeviceState *dev)
>>> return g_strdup(path);
>>> }
>>>
>>> -static int ide_qdev_init(DeviceState *qdev)
>>> +static void ide_qdev_realize(DeviceState *qdev, Error **errp)
>>> {
>>> IDEDevice *dev = IDE_DEVICE(qdev);
>>> IDEDeviceClass *dc = IDE_DEVICE_GET_CLASS(dev);
>>> @@ -91,34 +91,31 @@ static int ide_qdev_init(DeviceState *qdev)
>>> }
>>>
>>> if (dev->unit >= bus->max_units) {
>>> - error_report("Can't create IDE unit %d, bus supports only %d units",
>>> + error_setg(errp, "Can't create IDE unit %d, bus supports only %d units",
>>> dev->unit, bus->max_units);
>>> - goto err;
>>> + return;
>>> }
>>>
>>> switch (dev->unit) {
>>> case 0:
>>> if (bus->master) {
>>> - error_report("IDE unit %d is in use", dev->unit);
>>> - goto err;
>>> + error_setg(errp, "IDE unit %d is in use", dev->unit);
>>> + return;
>>> }
>>> bus->master = dev;
>>> break;
>>> case 1:
>>> if (bus->slave) {
>>> - error_report("IDE unit %d is in use", dev->unit);
>>> - goto err;
>>> + error_setg(errp, "IDE unit %d is in use", dev->unit);
>>> + return;
>>> }
>>> bus->slave = dev;
>>> break;
>>> default:
>>> - error_report("Invalid IDE unit %d", dev->unit);
>>> - goto err;
>>> + error_setg(errp, "Invalid IDE unit %d", dev->unit);
>>> + return;
>>> }
>>> - return dc->init(dev);
>>> -
>>> -err:
>>> - return -1;
>>> + dc->realize(dev, errp);
>>> }
>>>
>>> IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
>>> @@ -159,7 +156,7 @@ typedef struct IDEDrive {
>>> IDEDevice dev;
>>> } IDEDrive;
>>>
>>> -static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>> +static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
>>> {
>>> IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
>>> IDEState *s = bus->ifs + dev->unit;
>>> @@ -168,8 +165,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>>
>>> if (!dev->conf.blk) {
>>> if (kind != IDE_CD) {
>>> - error_report("No drive specified");
>>> - return -1;
>>> + error_setg(errp, "No drive specified");
>>> + return;
>>> } else {
>>> /* Anonymous BlockBackend for an empty drive */
>>> dev->conf.blk = blk_new(0, BLK_PERM_ALL);
>>> @@ -182,36 +179,36 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>> dev->conf.discard_granularity = 512;
>>> } else if (dev->conf.discard_granularity &&
>>> dev->conf.discard_granularity != 512) {
>>> - error_report("discard_granularity must be 512 for ide");
>>> - return -1;
>>> + error_setg(errp, "discard_granularity must be 512 for ide");
>>> + return;
>>> }
>>>
>>> blkconf_blocksizes(&dev->conf);
>>> if (dev->conf.logical_block_size != 512) {
>>> - error_report("logical_block_size must be 512 for IDE");
>>> - return -1;
>>> + error_setg(errp, "logical_block_size must be 512 for IDE");
>>> + return;
>>> }
>>>
>>> blkconf_serial(&dev->conf, &dev->serial);
>>> if (kind != IDE_CD) {
>>> blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255, &err);
>>> if (err) {
>>> - error_report_err(err);
>>> - return -1;
>>> + error_propagate(errp, err);
>>> + return;
>>> }
>>> }
>>> blkconf_apply_backend_options(&dev->conf, kind == IDE_CD, kind != IDE_CD,
>>> &err);
>>> if (err) {
>>> - error_report_err(err);
>>> - return -1;
>>> + error_propagate(errp, err);
>>> + return;
>>> }
>>>
>>> if (ide_init_drive(s, dev->conf.blk, kind,
>>> dev->version, dev->serial, dev->model, dev->wwn,
>>> dev->conf.cyls, dev->conf.heads, dev->conf.secs,
>>> - dev->chs_trans) < 0) {
>>> - return -1;
>>> + dev->chs_trans, errp) < 0) {
>>> + return;
>>> }
>>>
>>> if (!dev->version) {
>>> @@ -223,8 +220,6 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>>
>>> add_boot_device_path(dev->conf.bootindex, &dev->qdev,
>>> dev->unit ? "/disk@1" : "/disk@0");
>>> -
>>> - return 0;
>>> }
>>>
>>> static void ide_dev_get_bootindex(Object *obj, Visitor *v, const char *name,
>>> @@ -270,17 +265,17 @@ static void ide_dev_instance_init(Object *obj)
>>> object_property_set_int(obj, -1, "bootindex", NULL);
>>> }
>>>
>>> -static int ide_hd_initfn(IDEDevice *dev)
>>> +static void ide_hd_realize(IDEDevice *dev, Error **errp)
>>
>> why rename these but not ide_dev_initfn ? I guess because it's not
>> directly referenced as being a realize function.
>>
>>> {
>>> - return ide_dev_initfn(dev, IDE_HD);
>>> + ide_dev_initfn(dev, IDE_HD, errp);
>>> }
>>>
>>> -static int ide_cd_initfn(IDEDevice *dev)
>>> +static void ide_cd_realize(IDEDevice *dev, Error **errp)
>>> {
>>> - return ide_dev_initfn(dev, IDE_CD);
>>> + ide_dev_initfn(dev, IDE_CD, errp);
>>> }
>>>
>>> -static int ide_drive_initfn(IDEDevice *dev)
>>> +static void ide_drive_realize(IDEDevice *dev, Error **errp)
>>> {
>>> DriveInfo *dinfo = NULL;
>>>
>>> @@ -288,7 +283,7 @@ static int ide_drive_initfn(IDEDevice *dev)
>>> dinfo = blk_legacy_dinfo(dev->conf.blk);
>>> }
>>>
>>> - return ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD);
>>> + ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD, errp);
>>> }
>>>
>>> #define DEFINE_IDE_DEV_PROPERTIES() \
>>> @@ -311,10 +306,11 @@ static void ide_hd_class_init(ObjectClass *klass, void *data)
>>> {
>>> DeviceClass *dc = DEVICE_CLASS(klass);
>>> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
>>> - k->init = ide_hd_initfn;
>>> +
>>> + k->realize = ide_hd_realize;
>>> dc->fw_name = "drive";
>>> - dc->desc = "virtual IDE disk";
>>> - dc->props = ide_hd_properties;
>>> + dc->desc = "virtual IDE disk";
>>> + dc->props = ide_hd_properties;
>>> }
>>>
>>> static const TypeInfo ide_hd_info = {
>>> @@ -333,10 +329,11 @@ static void ide_cd_class_init(ObjectClass *klass, void *data)
>>> {
>>> DeviceClass *dc = DEVICE_CLASS(klass);
>>> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
>>> - k->init = ide_cd_initfn;
>>> +
>>> + k->realize = ide_cd_realize;
>>> dc->fw_name = "drive";
>>> - dc->desc = "virtual IDE CD-ROM";
>>> - dc->props = ide_cd_properties;
>>> + dc->desc = "virtual IDE CD-ROM";
>>> + dc->props = ide_cd_properties;
>>> }
>>>
>>> static const TypeInfo ide_cd_info = {
>>> @@ -355,10 +352,11 @@ static void ide_drive_class_init(ObjectClass *klass, void *data)
>>> {
>>> DeviceClass *dc = DEVICE_CLASS(klass);
>>> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
>>> - k->init = ide_drive_initfn;
>>> +
>>> + k->realize = ide_drive_realize;
>>> dc->fw_name = "drive";
>>> - dc->desc = "virtual IDE disk or CD-ROM (legacy)";
>>> - dc->props = ide_drive_properties;
>>> + dc->desc = "virtual IDE disk or CD-ROM (legacy)";
>>> + dc->props = ide_drive_properties;
>>> }
>>>
>>> static const TypeInfo ide_drive_info = {
>>> @@ -371,7 +369,7 @@ static const TypeInfo ide_drive_info = {
>>> static void ide_device_class_init(ObjectClass *klass, void *data)
>>> {
>>> DeviceClass *k = DEVICE_CLASS(klass);
>>> - k->init = ide_qdev_init;
>>> + k->realize = ide_qdev_realize;
>>> set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
>>> k->bus_type = TYPE_IDE_BUS;
>>> k->props = ide_props;
>>> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
>>> index 482a951..5ebb4c9 100644
>>> --- a/include/hw/ide/internal.h
>>> +++ b/include/hw/ide/internal.h
>>> @@ -12,6 +12,7 @@
>>> #include "sysemu/sysemu.h"
>>> #include "hw/block/block.h"
>>> #include "block/scsi.h"
>>> +#include "qapi/error.h"
>>>
>>> /* debug IDE devices */
>>> //#define DEBUG_IDE
>>> @@ -495,7 +496,7 @@ struct IDEBus {
>>>
>>> typedef struct IDEDeviceClass {
>>> DeviceClass parent_class;
>>> - int (*init)(IDEDevice *dev);
>>> + void (*realize)(IDEDevice *dev, Error **errp);
>>> } IDEDeviceClass;
>>>
>>> struct IDEDevice {
>>> @@ -605,7 +606,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>>> const char *version, const char *serial, const char *model,
>>> uint64_t wwn,
>>> uint32_t cylinders, uint32_t heads, uint32_t secs,
>>> - int chs_trans);
>>> + int chs_trans, Error **errp);
>>> void ide_init2(IDEBus *bus, qemu_irq irq);
>>> void ide_exit(IDEState *s);
>>> void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>>>
>>
>> Only matters of style; seems mechanically OK.
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
>
> Whoops, rescind that; this causes some test failures in test 051. You
> need to update the test output in 051 to match the new failure modes.
Thanks for the review. I will.
Thanks,
Mao
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/6] hw/ide: Convert DeviceClass init to realize
2017-09-15 21:35 ` John Snow
2017-09-15 21:42 ` Eric Blake
2017-09-15 22:03 ` [Qemu-devel] [Qemu-block] " John Snow
@ 2017-09-18 1:35 ` Mao Zhongyi
2 siblings, 0 replies; 17+ messages in thread
From: Mao Zhongyi @ 2017-09-18 1:35 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block; +Cc: Markus Armbruster
On 09/16/2017 05:35 AM, John Snow wrote:
>
>
> On 08/04/2017 06:26 AM, Mao Zhongyi wrote:
>> Replace init with realize in IDEDeviceClass, which has errp
>> as a parameter. So all the implementations now use error_setg
>> instead of error_report for reporting error.
>>
>> Cc: John Snow <jsnow@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>>
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> hw/ide/core.c | 7 ++--
>> hw/ide/qdev.c | 86 +++++++++++++++++++++++------------------------
>> include/hw/ide/internal.h | 5 +--
>> 3 files changed, 49 insertions(+), 49 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 0b48b64..7c86fc7 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -34,6 +34,7 @@
>> #include "hw/block/block.h"
>> #include "sysemu/block-backend.h"
>> #include "qemu/cutils.h"
>> +#include "qemu/error-report.h"
>>
>> #include "hw/ide/internal.h"
>>
>> @@ -2398,7 +2399,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>> const char *version, const char *serial, const char *model,
>> uint64_t wwn,
>> uint32_t cylinders, uint32_t heads, uint32_t secs,
>> - int chs_trans)
>> + int chs_trans, Error **errp)
>
> this function now requires an additional invariant, which is that we
> must return -1 AND set errp. Probably wisest to just get rid of the
> return code so that we don't accidentally goof this up in the future.
>
> I think Markus has had some guidance on this in the past, but admittedly
> I can't remember his preference.
>
>> {
>> uint64_t nb_sectors;
>>
>> @@ -2423,11 +2424,11 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>> blk_set_guest_block_size(blk, 2048);
>> } else {
>> if (!blk_is_inserted(s->blk)) {
>> - error_report("Device needs media, but drive is empty");
>> + error_setg(errp, "Device needs media, but drive is empty");
>> return -1;
>> }
>> if (blk_is_read_only(blk)) {
>> - error_report("Can't use a read-only drive");
>> + error_setg(errp, "Can't use a read-only drive");
>> return -1;
>> }
>> blk_set_dev_ops(blk, &ide_hd_block_ops, s);
>> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
>> index cc2f5bd..d60ac25 100644
>> --- a/hw/ide/qdev.c
>> +++ b/hw/ide/qdev.c
>> @@ -80,7 +80,7 @@ static char *idebus_get_fw_dev_path(DeviceState *dev)
>> return g_strdup(path);
>> }
>>
>> -static int ide_qdev_init(DeviceState *qdev)
>> +static void ide_qdev_realize(DeviceState *qdev, Error **errp)
>> {
>> IDEDevice *dev = IDE_DEVICE(qdev);
>> IDEDeviceClass *dc = IDE_DEVICE_GET_CLASS(dev);
>> @@ -91,34 +91,31 @@ static int ide_qdev_init(DeviceState *qdev)
>> }
>>
>> if (dev->unit >= bus->max_units) {
>> - error_report("Can't create IDE unit %d, bus supports only %d units",
>> + error_setg(errp, "Can't create IDE unit %d, bus supports only %d units",
>> dev->unit, bus->max_units);
>> - goto err;
>> + return;
>> }
>>
>> switch (dev->unit) {
>> case 0:
>> if (bus->master) {
>> - error_report("IDE unit %d is in use", dev->unit);
>> - goto err;
>> + error_setg(errp, "IDE unit %d is in use", dev->unit);
>> + return;
>> }
>> bus->master = dev;
>> break;
>> case 1:
>> if (bus->slave) {
>> - error_report("IDE unit %d is in use", dev->unit);
>> - goto err;
>> + error_setg(errp, "IDE unit %d is in use", dev->unit);
>> + return;
>> }
>> bus->slave = dev;
>> break;
>> default:
>> - error_report("Invalid IDE unit %d", dev->unit);
>> - goto err;
>> + error_setg(errp, "Invalid IDE unit %d", dev->unit);
>> + return;
>> }
>> - return dc->init(dev);
>> -
>> -err:
>> - return -1;
>> + dc->realize(dev, errp);
>> }
>>
>> IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
>> @@ -159,7 +156,7 @@ typedef struct IDEDrive {
>> IDEDevice dev;
>> } IDEDrive;
>>
>> -static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>> +static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
>> {
>> IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
>> IDEState *s = bus->ifs + dev->unit;
>> @@ -168,8 +165,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>
>> if (!dev->conf.blk) {
>> if (kind != IDE_CD) {
>> - error_report("No drive specified");
>> - return -1;
>> + error_setg(errp, "No drive specified");
>> + return;
>> } else {
>> /* Anonymous BlockBackend for an empty drive */
>> dev->conf.blk = blk_new(0, BLK_PERM_ALL);
>> @@ -182,36 +179,36 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>> dev->conf.discard_granularity = 512;
>> } else if (dev->conf.discard_granularity &&
>> dev->conf.discard_granularity != 512) {
>> - error_report("discard_granularity must be 512 for ide");
>> - return -1;
>> + error_setg(errp, "discard_granularity must be 512 for ide");
>> + return;
>> }
>>
>> blkconf_blocksizes(&dev->conf);
>> if (dev->conf.logical_block_size != 512) {
>> - error_report("logical_block_size must be 512 for IDE");
>> - return -1;
>> + error_setg(errp, "logical_block_size must be 512 for IDE");
>> + return;
>> }
>>
>> blkconf_serial(&dev->conf, &dev->serial);
>> if (kind != IDE_CD) {
>> blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255, &err);
>> if (err) {
>> - error_report_err(err);
>> - return -1;
>> + error_propagate(errp, err);
>> + return;
>> }
>> }
>> blkconf_apply_backend_options(&dev->conf, kind == IDE_CD, kind != IDE_CD,
>> &err);
>> if (err) {
>> - error_report_err(err);
>> - return -1;
>> + error_propagate(errp, err);
>> + return;
>> }
>>
>> if (ide_init_drive(s, dev->conf.blk, kind,
>> dev->version, dev->serial, dev->model, dev->wwn,
>> dev->conf.cyls, dev->conf.heads, dev->conf.secs,
>> - dev->chs_trans) < 0) {
>> - return -1;
>> + dev->chs_trans, errp) < 0) {
>> + return;
>> }
>>
>> if (!dev->version) {
>> @@ -223,8 +220,6 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>>
>> add_boot_device_path(dev->conf.bootindex, &dev->qdev,
>> dev->unit ? "/disk@1" : "/disk@0");
>> -
>> - return 0;
>> }
>>
>> static void ide_dev_get_bootindex(Object *obj, Visitor *v, const char *name,
>> @@ -270,17 +265,17 @@ static void ide_dev_instance_init(Object *obj)
>> object_property_set_int(obj, -1, "bootindex", NULL);
>> }
>>
>> -static int ide_hd_initfn(IDEDevice *dev)
>> +static void ide_hd_realize(IDEDevice *dev, Error **errp)
>
> why rename these but not ide_dev_initfn ? I guess because it's not
> directly referenced as being a realize function.
Hi, John
Yes, but not exactly. Because the realize function used to initialize the device
can only accept two arguments, but ide_dev_initfn has three. If it is renamed to
realize, although it is not wrong, but it looks puzzled.
Thanks,
Mao
>
>> {
>> - return ide_dev_initfn(dev, IDE_HD);
>> + ide_dev_initfn(dev, IDE_HD, errp);
>> }
>>
>> -static int ide_cd_initfn(IDEDevice *dev)
>> +static void ide_cd_realize(IDEDevice *dev, Error **errp)
>> {
>> - return ide_dev_initfn(dev, IDE_CD);
>> + ide_dev_initfn(dev, IDE_CD, errp);
>> }
>>
>> -static int ide_drive_initfn(IDEDevice *dev)
>> +static void ide_drive_realize(IDEDevice *dev, Error **errp)
>> {
>> DriveInfo *dinfo = NULL;
>>
>> @@ -288,7 +283,7 @@ static int ide_drive_initfn(IDEDevice *dev)
>> dinfo = blk_legacy_dinfo(dev->conf.blk);
>> }
>>
>> - return ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD);
>> + ide_dev_initfn(dev, dinfo && dinfo->media_cd ? IDE_CD : IDE_HD, errp);
>> }
>>
>> #define DEFINE_IDE_DEV_PROPERTIES() \
>> @@ -311,10 +306,11 @@ static void ide_hd_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
>> - k->init = ide_hd_initfn;
>> +
>> + k->realize = ide_hd_realize;
>> dc->fw_name = "drive";
>> - dc->desc = "virtual IDE disk";
>> - dc->props = ide_hd_properties;
>> + dc->desc = "virtual IDE disk";
>> + dc->props = ide_hd_properties;
>> }
>>
>> static const TypeInfo ide_hd_info = {
>> @@ -333,10 +329,11 @@ static void ide_cd_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
>> - k->init = ide_cd_initfn;
>> +
>> + k->realize = ide_cd_realize;
>> dc->fw_name = "drive";
>> - dc->desc = "virtual IDE CD-ROM";
>> - dc->props = ide_cd_properties;
>> + dc->desc = "virtual IDE CD-ROM";
>> + dc->props = ide_cd_properties;
>> }
>>
>> static const TypeInfo ide_cd_info = {
>> @@ -355,10 +352,11 @@ static void ide_drive_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> IDEDeviceClass *k = IDE_DEVICE_CLASS(klass);
>> - k->init = ide_drive_initfn;
>> +
>> + k->realize = ide_drive_realize;
>> dc->fw_name = "drive";
>> - dc->desc = "virtual IDE disk or CD-ROM (legacy)";
>> - dc->props = ide_drive_properties;
>> + dc->desc = "virtual IDE disk or CD-ROM (legacy)";
>> + dc->props = ide_drive_properties;
>> }
>>
>> static const TypeInfo ide_drive_info = {
>> @@ -371,7 +369,7 @@ static const TypeInfo ide_drive_info = {
>> static void ide_device_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *k = DEVICE_CLASS(klass);
>> - k->init = ide_qdev_init;
>> + k->realize = ide_qdev_realize;
>> set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
>> k->bus_type = TYPE_IDE_BUS;
>> k->props = ide_props;
>> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
>> index 482a951..5ebb4c9 100644
>> --- a/include/hw/ide/internal.h
>> +++ b/include/hw/ide/internal.h
>> @@ -12,6 +12,7 @@
>> #include "sysemu/sysemu.h"
>> #include "hw/block/block.h"
>> #include "block/scsi.h"
>> +#include "qapi/error.h"
>>
>> /* debug IDE devices */
>> //#define DEBUG_IDE
>> @@ -495,7 +496,7 @@ struct IDEBus {
>>
>> typedef struct IDEDeviceClass {
>> DeviceClass parent_class;
>> - int (*init)(IDEDevice *dev);
>> + void (*realize)(IDEDevice *dev, Error **errp);
>> } IDEDeviceClass;
>>
>> struct IDEDevice {
>> @@ -605,7 +606,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
>> const char *version, const char *serial, const char *model,
>> uint64_t wwn,
>> uint32_t cylinders, uint32_t heads, uint32_t secs,
>> - int chs_trans);
>> + int chs_trans, Error **errp);
>> void ide_init2(IDEBus *bus, qemu_irq irq);
>> void ide_exit(IDEState *s);
>> void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>>
>
> Only matters of style; seems mechanically OK.
>
> Reviewed-by: John Snow <jsnow@redhat.com>
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v2 2/6] hw/block/fdc: Convert to realize
2017-08-04 10:26 [Qemu-devel] [PATCH v2 0/6] Convert to realize and improve error handling Mao Zhongyi
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 1/6] hw/ide: Convert DeviceClass init to realize Mao Zhongyi
@ 2017-08-04 10:26 ` Mao Zhongyi
2017-09-15 22:12 ` John Snow
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 3/6] hw/block/nvme: " Mao Zhongyi
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Mao Zhongyi @ 2017-08-04 10:26 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: John Snow, Kevin Wolf, Max Reitz, Markus Armbruster
Convert floppy_drive_init() to realize and rename it to
floppy_drive_realize().
Cc: John Snow <jsnow@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/block/fdc.c | 33 ++++++++++++++++-----------------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4011290..02f4a46 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -517,7 +517,7 @@ static Property floppy_drive_properties[] = {
DEFINE_PROP_END_OF_LIST(),
};
-static int floppy_drive_init(DeviceState *qdev)
+static void floppy_drive_realize(DeviceState *qdev, Error **errp)
{
FloppyDrive *dev = FLOPPY_DRIVE(qdev);
FloppyBus *bus = FLOPPY_BUS(qdev->parent_bus);
@@ -535,15 +535,15 @@ static int floppy_drive_init(DeviceState *qdev)
}
if (dev->unit >= MAX_FD) {
- error_report("Can't create floppy unit %d, bus supports only %d units",
- dev->unit, MAX_FD);
- return -1;
+ error_setg(errp, "Can't create floppy unit %d, bus supports "
+ "only %d units", dev->unit, MAX_FD);
+ return;
}
drive = get_drv(bus->fdc, dev->unit);
if (drive->blk) {
- error_report("Floppy unit %d is in use", dev->unit);
- return -1;
+ error_setg(errp, "Floppy unit %d is in use", dev->unit);
+ return;
}
if (!dev->conf.blk) {
@@ -557,8 +557,9 @@ static int floppy_drive_init(DeviceState *qdev)
if (dev->conf.logical_block_size != 512 ||
dev->conf.physical_block_size != 512)
{
- error_report("Physical and logical block size must be 512 for floppy");
- return -1;
+ error_setg(errp, "Physical and logical block size must "
+ "be 512 for floppy");
+ return;
}
/* rerror/werror aren't supported by fdc and therefore not even registered
@@ -570,20 +571,20 @@ static int floppy_drive_init(DeviceState *qdev)
blkconf_apply_backend_options(&dev->conf, blk_is_read_only(dev->conf.blk),
false, &local_err);
if (local_err) {
- error_report_err(local_err);
- return -1;
+ error_propagate(errp, local_err);
+ return;
}
/* 'enospc' is the default for -drive, 'report' is what blk_new() gives us
* for empty drives. */
if (blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC &&
blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_REPORT) {
- error_report("fdc doesn't support drive option werror");
- return -1;
+ error_setg(errp, "fdc doesn't support drive option werror");
+ return;
}
if (blk_get_on_error(dev->conf.blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
- error_report("fdc doesn't support drive option rerror");
- return -1;
+ error_setg(errp, "fdc doesn't support drive option rerror");
+ return;
}
drive->conf = &dev->conf;
@@ -599,14 +600,12 @@ static int floppy_drive_init(DeviceState *qdev)
dev->type = drive->drive;
fd_revalidate(drive);
-
- return 0;
}
static void floppy_drive_class_init(ObjectClass *klass, void *data)
{
DeviceClass *k = DEVICE_CLASS(klass);
- k->init = floppy_drive_init;
+ k->realize = floppy_drive_realize;
set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
k->bus_type = TYPE_FLOPPY_BUS;
k->props = floppy_drive_properties;
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v2 2/6] hw/block/fdc: Convert to realize
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 2/6] hw/block/fdc: Convert " Mao Zhongyi
@ 2017-09-15 22:12 ` John Snow
0 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2017-09-15 22:12 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel, qemu-block, Kevin Wolf
Cc: Markus Armbruster, Max Reitz
On 08/04/2017 06:26 AM, Mao Zhongyi wrote:
> Convert floppy_drive_init() to realize and rename it to
> floppy_drive_realize().
>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Similar to the first patch, you need to visit the test output for 172
and update it to match accordingly. Should be a trivial fix.
--js
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v2 3/6] hw/block/nvme: Convert to realize
2017-08-04 10:26 [Qemu-devel] [PATCH v2 0/6] Convert to realize and improve error handling Mao Zhongyi
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 1/6] hw/ide: Convert DeviceClass init to realize Mao Zhongyi
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 2/6] hw/block/fdc: Convert " Mao Zhongyi
@ 2017-08-04 10:26 ` Mao Zhongyi
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 4/6] hw/block: Fix the return type Mao Zhongyi
` (2 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Mao Zhongyi @ 2017-08-04 10:26 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: John Snow, Keith Busch, Kevin Wolf, Max Reitz, Markus Armbruster
Convert nvme_init() to realize and rename it to nvme_realize().
Cc: John Snow <jsnow@redhat.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/block/nvme.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 6071dc1..43c60ab 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -920,7 +920,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
},
};
-static int nvme_init(PCIDevice *pci_dev)
+static void nvme_realize(PCIDevice *pci_dev, Error **errp)
{
NvmeCtrl *n = NVME(pci_dev);
NvmeIdCtrl *id = &n->id_ctrl;
@@ -931,24 +931,27 @@ static int nvme_init(PCIDevice *pci_dev)
Error *local_err = NULL;
if (!n->conf.blk) {
- return -1;
+ error_setg(errp, "drive property not set");
+ return;
}
bs_size = blk_getlength(n->conf.blk);
if (bs_size < 0) {
- return -1;
+ error_setg(errp, "could not get backing file size");
+ return;
}
blkconf_serial(&n->conf, &n->serial);
if (!n->serial) {
- return -1;
+ error_setg(errp, "serial property not set");
+ return;
}
blkconf_blocksizes(&n->conf);
blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
false, &local_err);
if (local_err) {
- error_report_err(local_err);
- return -1;
+ error_propagate(errp, local_err);
+ return;
}
pci_conf = pci_dev->config;
@@ -1046,7 +1049,6 @@ static int nvme_init(PCIDevice *pci_dev)
cpu_to_le64(n->ns_size >>
id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds);
}
- return 0;
}
static void nvme_exit(PCIDevice *pci_dev)
@@ -1081,7 +1083,7 @@ static void nvme_class_init(ObjectClass *oc, void *data)
DeviceClass *dc = DEVICE_CLASS(oc);
PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
- pc->init = nvme_init;
+ pc->realize = nvme_realize;
pc->exit = nvme_exit;
pc->class_id = PCI_CLASS_STORAGE_EXPRESS;
pc->vendor_id = PCI_VENDOR_ID_INTEL;
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH v2 4/6] hw/block: Fix the return type
2017-08-04 10:26 [Qemu-devel] [PATCH v2 0/6] Convert to realize and improve error handling Mao Zhongyi
` (2 preceding siblings ...)
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 3/6] hw/block/nvme: " Mao Zhongyi
@ 2017-08-04 10:26 ` Mao Zhongyi
2017-08-04 12:53 ` Stefan Hajnoczi
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 5/6] hw/block: Use errp directly rather than local_err Mao Zhongyi
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 6/6] dev-storage: Fix the unusual function name Mao Zhongyi
5 siblings, 1 reply; 17+ messages in thread
From: Mao Zhongyi @ 2017-08-04 10:26 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: John Snow, Kevin Wolf, Max Reitz, Stefan Hajnoczi
When the function no success value to transmit, it usually make the
function return void. It has turned out not to be a success, because
it means that the extra local_err variable and error_propagate() will
be needed. It leads to cumbersome code, therefore, transmit success/
failure in the return value is worth.
So fix the return type of blkconf_apply_backend_options(),
blkconf_geometry() and virtio_blk_data_plane_create() to avoid it.
Cc: John Snow <jsnow@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/block/block.c | 15 +++++++++------
hw/block/dataplane/virtio-blk.c | 12 +++++++-----
hw/block/dataplane/virtio-blk.h | 2 +-
include/hw/block/block.h | 4 ++--
4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/hw/block/block.c b/hw/block/block.c
index 27878d0..b0269c8 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -51,7 +51,7 @@ void blkconf_blocksizes(BlockConf *conf)
}
}
-void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
+bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
bool resizable, Error **errp)
{
BlockBackend *blk = conf->blk;
@@ -76,7 +76,7 @@ void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
ret = blk_set_perm(blk, perm, shared_perm, errp);
if (ret < 0) {
- return;
+ return false;
}
switch (conf->wce) {
@@ -99,9 +99,11 @@ void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
blk_set_enable_write_cache(blk, wce);
blk_set_on_error(blk, rerror, werror);
+
+ return true;
}
-void blkconf_geometry(BlockConf *conf, int *ptrans,
+bool blkconf_geometry(BlockConf *conf, int *ptrans,
unsigned cyls_max, unsigned heads_max, unsigned secs_max,
Error **errp)
{
@@ -129,15 +131,16 @@ void blkconf_geometry(BlockConf *conf, int *ptrans,
if (conf->cyls || conf->heads || conf->secs) {
if (conf->cyls < 1 || conf->cyls > cyls_max) {
error_setg(errp, "cyls must be between 1 and %u", cyls_max);
- return;
+ return false;
}
if (conf->heads < 1 || conf->heads > heads_max) {
error_setg(errp, "heads must be between 1 and %u", heads_max);
- return;
+ return false;
}
if (conf->secs < 1 || conf->secs > secs_max) {
error_setg(errp, "secs must be between 1 and %u", secs_max);
- return;
+ return false;
}
}
+ return true;
}
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 5556f0e..f6fc639 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -76,7 +76,7 @@ static void notify_guest_bh(void *opaque)
}
/* Context: QEMU global mutex held */
-void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
+bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
VirtIOBlockDataPlane **dataplane,
Error **errp)
{
@@ -91,11 +91,11 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
error_setg(errp,
"device is incompatible with iothread "
"(transport does not support notifiers)");
- return;
+ return false;
}
if (!virtio_device_ioeventfd_enabled(vdev)) {
error_setg(errp, "ioeventfd is required for iothread");
- return;
+ return false;
}
/* If dataplane is (re-)enabled while the guest is running there could
@@ -103,12 +103,12 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
*/
if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
error_prepend(errp, "cannot start virtio-blk dataplane: ");
- return;
+ return false;
}
}
/* Don't try if transport does not support notifiers. */
if (!virtio_device_ioeventfd_enabled(vdev)) {
- return;
+ return false;
}
s = g_new0(VirtIOBlockDataPlane, 1);
@@ -126,6 +126,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
+
+ return true;
}
/* Context: QEMU global mutex held */
diff --git a/hw/block/dataplane/virtio-blk.h b/hw/block/dataplane/virtio-blk.h
index db3f47b..5e18bb9 100644
--- a/hw/block/dataplane/virtio-blk.h
+++ b/hw/block/dataplane/virtio-blk.h
@@ -19,7 +19,7 @@
typedef struct VirtIOBlockDataPlane VirtIOBlockDataPlane;
-void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
+bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
VirtIOBlockDataPlane **dataplane,
Error **errp);
void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s);
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index f3f6e8e..64b9298 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -72,11 +72,11 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
/* Configuration helpers */
void blkconf_serial(BlockConf *conf, char **serial);
-void blkconf_geometry(BlockConf *conf, int *trans,
+bool blkconf_geometry(BlockConf *conf, int *trans,
unsigned cyls_max, unsigned heads_max, unsigned secs_max,
Error **errp);
void blkconf_blocksizes(BlockConf *conf);
-void blkconf_apply_backend_options(BlockConf *conf, bool readonly,
+bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
bool resizable, Error **errp);
/* Hard disk geometry */
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v2 4/6] hw/block: Fix the return type
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 4/6] hw/block: Fix the return type Mao Zhongyi
@ 2017-08-04 12:53 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2017-08-04 12:53 UTC (permalink / raw)
To: Mao Zhongyi; +Cc: qemu-devel, qemu-block, John Snow, Kevin Wolf, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]
On Fri, Aug 04, 2017 at 06:26:41PM +0800, Mao Zhongyi wrote:
> When the function no success value to transmit, it usually make the
> function return void. It has turned out not to be a success, because
> it means that the extra local_err variable and error_propagate() will
> be needed. It leads to cumbersome code, therefore, transmit success/
> failure in the return value is worth.
>
> So fix the return type of blkconf_apply_backend_options(),
> blkconf_geometry() and virtio_blk_data_plane_create() to avoid it.
>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> hw/block/block.c | 15 +++++++++------
> hw/block/dataplane/virtio-blk.c | 12 +++++++-----
> hw/block/dataplane/virtio-blk.h | 2 +-
> include/hw/block/block.h | 4 ++--
> 4 files changed, 19 insertions(+), 14 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v2 5/6] hw/block: Use errp directly rather than local_err
2017-08-04 10:26 [Qemu-devel] [PATCH v2 0/6] Convert to realize and improve error handling Mao Zhongyi
` (3 preceding siblings ...)
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 4/6] hw/block: Fix the return type Mao Zhongyi
@ 2017-08-04 10:26 ` Mao Zhongyi
2017-08-04 12:53 ` Stefan Hajnoczi
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 6/6] dev-storage: Fix the unusual function name Mao Zhongyi
5 siblings, 1 reply; 17+ messages in thread
From: Mao Zhongyi @ 2017-08-04 10:26 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: John Snow, Kevin Wolf, Max Reitz, Keith Busch, Stefan Hajnoczi,
Michael S. Tsirkin, Paolo Bonzini, Gerd Hoffmann,
Markus Armbruster
Pass the error message to errp directly rather than the local
variable local_err and propagate it to errp via error_propagate().
Cc: John Snow <jsnow@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/block/fdc.c | 17 ++++++-----------
hw/block/nvme.c | 8 +++-----
hw/block/virtio-blk.c | 17 +++++------------
hw/ide/qdev.c | 12 ++++--------
hw/scsi/scsi-disk.c | 13 ++++---------
hw/usb/dev-storage.c | 9 +++------
6 files changed, 25 insertions(+), 51 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 02f4a46..192a4aa 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -473,16 +473,13 @@ static void fd_revalidate(FDrive *drv)
static void fd_change_cb(void *opaque, bool load, Error **errp)
{
FDrive *drive = opaque;
- Error *local_err = NULL;
if (!load) {
blk_set_perm(drive->blk, 0, BLK_PERM_ALL, &error_abort);
} else {
- blkconf_apply_backend_options(drive->conf,
- blk_is_read_only(drive->blk), false,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (!blkconf_apply_backend_options(drive->conf,
+ blk_is_read_only(drive->blk),
+ false, errp)) {
return;
}
}
@@ -522,7 +519,6 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
FloppyDrive *dev = FLOPPY_DRIVE(qdev);
FloppyBus *bus = FLOPPY_BUS(qdev->parent_bus);
FDrive *drive;
- Error *local_err = NULL;
int ret;
if (dev->unit == -1) {
@@ -568,10 +564,9 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
dev->conf.rerror = BLOCKDEV_ON_ERROR_AUTO;
dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO;
- blkconf_apply_backend_options(&dev->conf, blk_is_read_only(dev->conf.blk),
- false, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (!blkconf_apply_backend_options(&dev->conf,
+ blk_is_read_only(dev->conf.blk),
+ false, errp)) {
return;
}
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 43c60ab..1856053 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -928,7 +928,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
int i;
int64_t bs_size;
uint8_t *pci_conf;
- Error *local_err = NULL;
if (!n->conf.blk) {
error_setg(errp, "drive property not set");
@@ -947,10 +946,9 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
return;
}
blkconf_blocksizes(&n->conf);
- blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
- false, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (!blkconf_apply_backend_options(&n->conf,
+ blk_is_read_only(n->conf.blk),
+ false, errp)) {
return;
}
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index b750bd8..2940337 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -911,7 +911,6 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VirtIOBlock *s = VIRTIO_BLK(dev);
VirtIOBlkConf *conf = &s->conf;
- Error *err = NULL;
unsigned i;
if (!conf->conf.blk) {
@@ -928,17 +927,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
}
blkconf_serial(&conf->conf, &conf->serial);
- blkconf_apply_backend_options(&conf->conf,
- blk_is_read_only(conf->conf.blk), true,
- &err);
- if (err) {
- error_propagate(errp, err);
+ if (!blkconf_apply_backend_options(&conf->conf,
+ blk_is_read_only(conf->conf.blk),
+ true, errp)) {
return;
}
s->original_wce = blk_enable_write_cache(conf->conf.blk);
- blkconf_geometry(&conf->conf, NULL, 65535, 255, 255, &err);
- if (err) {
- error_propagate(errp, err);
+ if (!blkconf_geometry(&conf->conf, NULL, 65535, 255, 255, errp)) {
return;
}
blkconf_blocksizes(&conf->conf);
@@ -953,9 +948,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
for (i = 0; i < conf->num_queues; i++) {
virtio_add_queue(vdev, 128, virtio_blk_handle_output);
}
- virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
- if (err != NULL) {
- error_propagate(errp, err);
+ if (!virtio_blk_data_plane_create(vdev, conf, &s->dataplane, errp)) {
virtio_cleanup(vdev);
return;
}
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index d60ac25..9677191 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -160,7 +160,6 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
{
IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
IDEState *s = bus->ifs + dev->unit;
- Error *err = NULL;
int ret;
if (!dev->conf.blk) {
@@ -191,16 +190,13 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
blkconf_serial(&dev->conf, &dev->serial);
if (kind != IDE_CD) {
- blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255, &err);
- if (err) {
- error_propagate(errp, err);
+ if (!blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255,
+ errp)) {
return;
}
}
- blkconf_apply_backend_options(&dev->conf, kind == IDE_CD, kind != IDE_CD,
- &err);
- if (err) {
- error_propagate(errp, err);
+ if (!blkconf_apply_backend_options(&dev->conf, kind == IDE_CD,
+ kind != IDE_CD, errp)) {
return;
}
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 5f1e5e8..64a7fdd 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2306,7 +2306,6 @@ static void scsi_disk_unit_attention_reported(SCSIDevice *dev)
static void scsi_realize(SCSIDevice *dev, Error **errp)
{
SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
- Error *err = NULL;
if (!s->qdev.conf.blk) {
error_setg(errp, "drive property not set");
@@ -2322,17 +2321,13 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
blkconf_serial(&s->qdev.conf, &s->serial);
blkconf_blocksizes(&s->qdev.conf);
if (dev->type == TYPE_DISK) {
- blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err);
- if (err) {
- error_propagate(errp, err);
+ if (!blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, errp)) {
return;
}
}
- blkconf_apply_backend_options(&dev->conf,
- blk_is_read_only(s->qdev.conf.blk),
- dev->type == TYPE_DISK, &err);
- if (err) {
- error_propagate(errp, err);
+ if (!blkconf_apply_backend_options(&dev->conf,
+ blk_is_read_only(s->qdev.conf.blk),
+ dev->type == TYPE_DISK, errp)) {
return;
}
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 8a61ec9..801f552 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -601,7 +601,6 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
MSDState *s = USB_STORAGE_DEV(dev);
BlockBackend *blk = s->conf.blk;
SCSIDevice *scsi_dev;
- Error *err = NULL;
if (!blk) {
error_setg(errp, "drive property not set");
@@ -610,9 +609,8 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
blkconf_serial(&s->conf, &dev->serial);
blkconf_blocksizes(&s->conf);
- blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk), true, &err);
- if (err) {
- error_propagate(errp, err);
+ if (!blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk),
+ true, errp)) {
return;
}
@@ -636,10 +634,9 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
&usb_msd_scsi_info_storage, NULL);
scsi_dev = scsi_bus_legacy_add_drive(&s->bus, blk, 0, !!s->removable,
s->conf.bootindex, dev->serial,
- &err);
+ errp);
blk_unref(blk);
if (!scsi_dev) {
- error_propagate(errp, err);
return;
}
usb_msd_handle_reset(dev);
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v2 5/6] hw/block: Use errp directly rather than local_err
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 5/6] hw/block: Use errp directly rather than local_err Mao Zhongyi
@ 2017-08-04 12:53 ` Stefan Hajnoczi
0 siblings, 0 replies; 17+ messages in thread
From: Stefan Hajnoczi @ 2017-08-04 12:53 UTC (permalink / raw)
To: Mao Zhongyi
Cc: qemu-devel, qemu-block, John Snow, Kevin Wolf, Max Reitz,
Keith Busch, Michael S. Tsirkin, Paolo Bonzini, Gerd Hoffmann,
Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]
On Fri, Aug 04, 2017 at 06:26:42PM +0800, Mao Zhongyi wrote:
> Pass the error message to errp directly rather than the local
> variable local_err and propagate it to errp via error_propagate().
>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> hw/block/fdc.c | 17 ++++++-----------
> hw/block/nvme.c | 8 +++-----
> hw/block/virtio-blk.c | 17 +++++------------
> hw/ide/qdev.c | 12 ++++--------
> hw/scsi/scsi-disk.c | 13 ++++---------
> hw/usb/dev-storage.c | 9 +++------
> 6 files changed, 25 insertions(+), 51 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v2 6/6] dev-storage: Fix the unusual function name
2017-08-04 10:26 [Qemu-devel] [PATCH v2 0/6] Convert to realize and improve error handling Mao Zhongyi
` (4 preceding siblings ...)
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 5/6] hw/block: Use errp directly rather than local_err Mao Zhongyi
@ 2017-08-04 10:26 ` Mao Zhongyi
2017-08-04 16:26 ` Philippe Mathieu-Daudé
5 siblings, 1 reply; 17+ messages in thread
From: Mao Zhongyi @ 2017-08-04 10:26 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: Gerd Hoffmann
The function name of usb_msd_{realize,unrealize}_*,
usb_msd_class_initfn_* are unusual. Rename it to
usb_msd_*_{realize,unrealize}, usb_msd_class_*_initfn.
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
hw/usb/dev-storage.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 801f552..2a05cd5 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -596,7 +596,7 @@ static void usb_msd_unrealize_storage(USBDevice *dev, Error **errp)
object_unref(OBJECT(&s->bus));
}
-static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
+static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
{
MSDState *s = USB_STORAGE_DEV(dev);
BlockBackend *blk = s->conf.blk;
@@ -643,14 +643,14 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
s->scsi_dev = scsi_dev;
}
-static void usb_msd_unrealize_bot(USBDevice *dev, Error **errp)
+static void usb_msd_bot_unrealize(USBDevice *dev, Error **errp)
{
MSDState *s = USB_STORAGE_DEV(dev);
object_unref(OBJECT(&s->bus));
}
-static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
+static void usb_msd_bot_realize(USBDevice *dev, Error **errp)
{
MSDState *s = USB_STORAGE_DEV(dev);
DeviceState *d = DEVICE(dev);
@@ -764,12 +764,12 @@ static void usb_msd_class_initfn_common(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_usb_msd;
}
-static void usb_msd_class_initfn_storage(ObjectClass *klass, void *data)
+static void usb_msd_class_storage_initfn(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
- uc->realize = usb_msd_realize_storage;
+ uc->realize = usb_msd_storage_realize;
uc->unrealize = usb_msd_unrealize_storage;
dc->props = msd_properties;
}
@@ -828,26 +828,26 @@ static void usb_msd_instance_init(Object *obj)
object_property_set_int(obj, -1, "bootindex", NULL);
}
-static void usb_msd_class_initfn_bot(ObjectClass *klass, void *data)
+static void usb_msd_class_bot_initfn(ObjectClass *klass, void *data)
{
USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
- uc->realize = usb_msd_realize_bot;
- uc->unrealize = usb_msd_unrealize_bot;
+ uc->realize = usb_msd_bot_realize;
+ uc->unrealize = usb_msd_bot_unrealize;
uc->attached_settable = true;
}
static const TypeInfo msd_info = {
.name = "usb-storage",
.parent = TYPE_USB_STORAGE,
- .class_init = usb_msd_class_initfn_storage,
+ .class_init = usb_msd_class_storage_initfn,
.instance_init = usb_msd_instance_init,
};
static const TypeInfo bot_info = {
.name = "usb-bot",
.parent = TYPE_USB_STORAGE,
- .class_init = usb_msd_class_initfn_bot,
+ .class_init = usb_msd_class_bot_initfn,
};
static void usb_msd_register_types(void)
--
2.9.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH v2 6/6] dev-storage: Fix the unusual function name
2017-08-04 10:26 ` [Qemu-devel] [PATCH v2 6/6] dev-storage: Fix the unusual function name Mao Zhongyi
@ 2017-08-04 16:26 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 17+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-08-04 16:26 UTC (permalink / raw)
To: Mao Zhongyi, qemu-devel, qemu-block; +Cc: Gerd Hoffmann
On 08/04/2017 07:26 AM, Mao Zhongyi wrote:
> The function name of usb_msd_{realize,unrealize}_*,
> usb_msd_class_initfn_* are unusual. Rename it to
> usb_msd_*_{realize,unrealize}, usb_msd_class_*_initfn.
>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/usb/dev-storage.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index 801f552..2a05cd5 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -596,7 +596,7 @@ static void usb_msd_unrealize_storage(USBDevice *dev, Error **errp)
> object_unref(OBJECT(&s->bus));
> }
>
> -static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
> +static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
> {
> MSDState *s = USB_STORAGE_DEV(dev);
> BlockBackend *blk = s->conf.blk;
> @@ -643,14 +643,14 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
> s->scsi_dev = scsi_dev;
> }
>
> -static void usb_msd_unrealize_bot(USBDevice *dev, Error **errp)
> +static void usb_msd_bot_unrealize(USBDevice *dev, Error **errp)
> {
> MSDState *s = USB_STORAGE_DEV(dev);
>
> object_unref(OBJECT(&s->bus));
> }
>
> -static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
> +static void usb_msd_bot_realize(USBDevice *dev, Error **errp)
> {
> MSDState *s = USB_STORAGE_DEV(dev);
> DeviceState *d = DEVICE(dev);
> @@ -764,12 +764,12 @@ static void usb_msd_class_initfn_common(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_usb_msd;
> }
>
> -static void usb_msd_class_initfn_storage(ObjectClass *klass, void *data)
> +static void usb_msd_class_storage_initfn(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
>
> - uc->realize = usb_msd_realize_storage;
> + uc->realize = usb_msd_storage_realize;
> uc->unrealize = usb_msd_unrealize_storage;
> dc->props = msd_properties;
> }
> @@ -828,26 +828,26 @@ static void usb_msd_instance_init(Object *obj)
> object_property_set_int(obj, -1, "bootindex", NULL);
> }
>
> -static void usb_msd_class_initfn_bot(ObjectClass *klass, void *data)
> +static void usb_msd_class_bot_initfn(ObjectClass *klass, void *data)
> {
> USBDeviceClass *uc = USB_DEVICE_CLASS(klass);
>
> - uc->realize = usb_msd_realize_bot;
> - uc->unrealize = usb_msd_unrealize_bot;
> + uc->realize = usb_msd_bot_realize;
> + uc->unrealize = usb_msd_bot_unrealize;
> uc->attached_settable = true;
> }
>
> static const TypeInfo msd_info = {
> .name = "usb-storage",
> .parent = TYPE_USB_STORAGE,
> - .class_init = usb_msd_class_initfn_storage,
> + .class_init = usb_msd_class_storage_initfn,
> .instance_init = usb_msd_instance_init,
> };
>
> static const TypeInfo bot_info = {
> .name = "usb-bot",
> .parent = TYPE_USB_STORAGE,
> - .class_init = usb_msd_class_initfn_bot,
> + .class_init = usb_msd_class_bot_initfn,
> };
>
> static void usb_msd_register_types(void)
>
^ permalink raw reply [flat|nested] 17+ messages in thread