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

[Fam Zheng <famz@redhat.com>]

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

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

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

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

> 
> > @@ -169,43 +168,40 @@ static int scsi_qdev_init(DeviceState *qdev)
> >              d = scsi_device_find(bus, dev->channel, dev->id, ++lun);
> >          } while (d && d->lun == lun && lun < bus->info->max_lun);
> >          if (d && d->lun == lun) {
> > -            error_report("no free lun");
> > -            goto err;
> > +            error_setg(errp, "no free lun");
> > +            return;
> >          }
> >          dev->lun = lun;
> >      } else {
> >          d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);
> >          assert(d);
> >          if (d->lun == dev->lun && dev != d) {
> > -            error_report("lun already used by '%s'", d->qdev.id);
> > -            goto err;
> > +            error_setg(errp, "lun already used by '%s'", d->qdev.id);
> > +            return;
> >          }
> >      }
> >  
> >      QTAILQ_INIT(&dev->requests);
> > -    rc = scsi_device_init(dev);
> > -    if (rc == 0) {
> > +    scsi_device_realize(dev, &local_err);
> > +    if (local_err) {
> >          dev->vmsentry = qemu_add_vm_change_state_handler(scsi_dma_restart_cb,
> >                                                           dev);
> > +        error_propagate(errp, local_err);
> >      }
> 
> Maybe I'm misunderstanding something, but it looks to me as if the
> handler was previously installed in case of success, and now it's only
> installed on failure?

Good catch! Will fix.

Thanks,
Fam