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

[Kevin Wolf]

Am 05.08.2014 um 11:11 hat Fam Zheng geschrieben:
> Replace "init/destroy" with "realize/unrealize" in SCSIDeviceClass,
> which has errp as a parameter. So all the implementations now uses
> error_setg instead of error_report for reporting error.
> 
> Also in lsi53c895a, report the error when initializing the if=scsi
> devices, before dropping it, because the callee's error_report is
> changed to error_segs.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  hw/scsi/lsi53c895a.c       |  2 ++
>  hw/scsi/scsi-bus.c         | 64 ++++++++++++++++++-------------------
>  hw/scsi/scsi-disk.c        | 78 
> ++++++++++++++++++++++++----------------------
>  hw/scsi/scsi-generic.c     | 37 +++++++++++-----------
>  include/hw/scsi/scsi.h     |  7 +++--
>  tests/qemu-iotests/051.out |  4 +--
>  6 files changed, 96 insertions(+), 96 deletions(-)
> 
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index 786d848..dbc98a0 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -19,6 +19,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/scsi/scsi.h"
>  #include "sysemu/dma.h"
> +#include "qemu/error-report.h"
>  
>  //#define DEBUG_LSI
>  //#define DEBUG_LSI_REG
> @@ -2121,6 +2122,7 @@ static int lsi_scsi_init(PCIDevice *dev)
>      if (!d->hotplugged) {
>          scsi_bus_legacy_handle_cmdline(&s->bus, &err);
>          if (err != NULL) {
> +            error_report("%s", error_get_pretty(err));
>              error_free(err);
>              return -1;
>          }

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

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

> @@ -169,43 +168,40 @@ static int scsi_qdev_init(DeviceState *qdev)
>              d = scsi_device_find(bus, dev->channel, dev->id, ++lun);
>          } while (d && d->lun == lun && lun < bus->info->max_lun);
>          if (d && d->lun == lun) {
> -            error_report("no free lun");
> -            goto err;
> +            error_setg(errp, "no free lun");
> +            return;
>          }
>          dev->lun = lun;
>      } else {
>          d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);
>          assert(d);
>          if (d->lun == dev->lun && dev != d) {
> -            error_report("lun already used by '%s'", d->qdev.id);
> -            goto err;
> +            error_setg(errp, "lun already used by '%s'", d->qdev.id);
> +            return;
>          }
>      }
>  
>      QTAILQ_INIT(&dev->requests);
> -    rc = scsi_device_init(dev);
> -    if (rc == 0) {
> +    scsi_device_realize(dev, &local_err);
> +    if (local_err) {
>          dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
>                                                           dev);
> +        error_propagate(errp, local_err);
>      }

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

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

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

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

Kevin