[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/4] hw: Propagate errors through qdev_prop_set_
|
From: |
Peter Crosthwaite |
|
Subject: |
Re: [Qemu-devel] [PATCH 2/4] hw: Propagate errors through qdev_prop_set_drive() |
|
Date: |
Sat, 21 Feb 2015 10:19:46 -0800 |
On Fri, Feb 20, 2015 at 11:19 AM, Markus Armbruster <address@hidden> wrote:
> Three kinds of callers:
>
> 1. On failure, report the error and abort
>
> Passing &error_abort does the job. No functional change.
>
> 2. On failure, report the error and exit()
>
> This is qdev_prop_set_drive_nofail(). Error reporting moves from
> qdev_prop_set_drive() to its caller. Because hiding away the error
> in the monitor right before exit() isn't helpful, replace
> qerror_report_err() by error_report_err(). Shouldn't make a
> difference, because qdev_prop_set_drive_nofail() should never be
> used in QMP context.
>
> 3. On failure, report the error and recover
>
> This is usb_msd_init() and scsi_bus_legacy_add_drive(). Error
> reporting and freeing the error object moves from
> qdev_prop_set_drive() to its callers.
>
> Because usb_msd_init() can't run in QMP context, replace
> qerror_report_err() by error_report_err() there.
>
> No functional change.
>
> scsi_bus_legacy_add_drive() calling qerror_report_err() is of
> course inappropriate, but this commit merely makes it more obvious.
> The next one will clean it up.
>
> Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Peter Crosthwaite <address@hidden>
> ---
> hw/arm/vexpress.c | 6 +++---
> hw/arm/virt.c | 6 +++---
> hw/block/pflash_cfi01.c | 4 ++--
> hw/block/pflash_cfi02.c | 4 ++--
> hw/core/qdev-properties-system.c | 22 ++++++++++------------
> hw/scsi/scsi-bus.c | 6 +++++-
> hw/usb/dev-storage.c | 7 +++++--
> include/hw/qdev-properties.h | 4 ++--
> 8 files changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 5933454..8496c16 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -515,9 +515,9 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base,
> const char *name,
> {
> DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
>
> - if (di && qdev_prop_set_drive(dev, "drive",
> - blk_by_legacy_dinfo(di))) {
> - abort();
> + if (di) {
> + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di),
> + &error_abort);
> }
>
> qdev_prop_set_uint32(dev, "num-blocks",
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 69f51ac..93b7605 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -522,9 +522,9 @@ static void create_one_flash(const char *name, hwaddr
> flashbase,
> DeviceState *dev = qdev_create(NULL, "cfi.pflash01");
> const uint64_t sectorlength = 256 * 1024;
>
> - if (dinfo && qdev_prop_set_drive(dev, "drive",
> - blk_by_legacy_dinfo(dinfo))) {
> - abort();
> + if (dinfo) {
> + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo),
> + &error_abort);
> }
>
> qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength);
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 89d380e..d282695 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -969,8 +969,8 @@ pflash_t *pflash_cfi01_register(hwaddr base,
> {
> DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
>
> - if (blk && qdev_prop_set_drive(dev, "drive", blk)) {
> - abort();
> + if (blk) {
> + qdev_prop_set_drive(dev, "drive", blk, &error_abort);
> }
> qdev_prop_set_uint32(dev, "num-blocks", nb_blocs);
> qdev_prop_set_uint64(dev, "sector-length", sector_len);
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 389b4aa..074a005 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -773,8 +773,8 @@ pflash_t *pflash_cfi02_register(hwaddr base,
> {
> DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH02);
>
> - if (blk && qdev_prop_set_drive(dev, "drive", blk)) {
> - abort();
> + if (blk) {
> + qdev_prop_set_drive(dev, "drive", blk, &error_abort);
> }
> qdev_prop_set_uint32(dev, "num-blocks", nb_blocs);
> qdev_prop_set_uint32(dev, "sector-length", sector_len);
> diff --git a/hw/core/qdev-properties-system.c
> b/hw/core/qdev-properties-system.c
> index a2e44bd..c413226 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -341,27 +341,25 @@ PropertyInfo qdev_prop_vlan = {
> .set = set_vlan,
> };
>
> -int qdev_prop_set_drive(DeviceState *dev, const char *name,
> - BlockBackend *value)
> +void qdev_prop_set_drive(DeviceState *dev, const char *name,
> + BlockBackend *value, Error **errp)
> {
> - Error *err = NULL;
> - object_property_set_str(OBJECT(dev),
> - value ? blk_name(value) : "", name, &err);
> - if (err) {
> - qerror_report_err(err);
> - error_free(err);
> - return -1;
> - }
> - return 0;
> + object_property_set_str(OBJECT(dev), value ? blk_name(value) : "",
> + name, errp);
> }
>
> void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name,
> BlockBackend *value)
> {
> - if (qdev_prop_set_drive(dev, name, value) < 0) {
> + Error *err = NULL;
> +
> + qdev_prop_set_drive(dev, name, value, &err);
> + if (err) {
> + error_report_err(err);
> exit(1);
> }
> }
> +
> void qdev_prop_set_chr(DeviceState *dev, const char *name,
> CharDriverState *value)
> {
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index f8de569..61c595f 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -7,6 +7,7 @@
> #include "sysemu/blockdev.h"
> #include "trace.h"
> #include "sysemu/dma.h"
> +#include "qapi/qmp/qerror.h"
>
> static char *scsibus_get_dev_path(DeviceState *dev);
> static char *scsibus_get_fw_dev_path(DeviceState *dev);
> @@ -242,7 +243,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus,
> BlockBackend *blk,
> if (serial && object_property_find(OBJECT(dev), "serial", NULL)) {
> qdev_prop_set_string(dev, "serial", serial);
> }
> - if (qdev_prop_set_drive(dev, "drive", blk) < 0) {
> + qdev_prop_set_drive(dev, "drive", blk, &err);
> + if (err) {
> + qerror_report_err(err);
> + error_free(err);
> error_setg(errp, "Setting drive property failed");
> object_unparent(OBJECT(dev));
> return NULL;
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index af2e1b9..4c0ef54 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -663,6 +663,7 @@ static void usb_msd_realize_bot(USBDevice *dev, Error
> **errp)
> static USBDevice *usb_msd_init(USBBus *bus, const char *filename)
> {
> static int nr=0;
> + Error *err = NULL;
> char id[8];
> QemuOpts *opts;
> DriveInfo *dinfo;
> @@ -706,8 +707,10 @@ static USBDevice *usb_msd_init(USBBus *bus, const char
> *filename)
>
> /* create guest device */
> dev = usb_create(bus, "usb-storage");
> - if (qdev_prop_set_drive(&dev->qdev, "drive",
> - blk_by_legacy_dinfo(dinfo)) < 0) {
> + qdev_prop_set_drive(&dev->qdev, "drive", blk_by_legacy_dinfo(dinfo),
> + &err);
> + if (err) {
> + error_report_err(err);
> object_unparent(OBJECT(dev));
> return NULL;
> }
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 57ee363..2d47c0c 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -168,8 +168,8 @@ void qdev_prop_set_uint64(DeviceState *dev, const char
> *name, uint64_t value);
> void qdev_prop_set_string(DeviceState *dev, const char *name, const char
> *value);
> void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState
> *value);
> void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState
> *value);
> -int qdev_prop_set_drive(DeviceState *dev, const char *name,
> - BlockBackend *value) QEMU_WARN_UNUSED_RESULT;
> +void qdev_prop_set_drive(DeviceState *dev, const char *name,
> + BlockBackend *value, Error **errp);
> void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name,
> BlockBackend *value);
> void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t
> *value);
> --
> 1.9.3
>
>