Re: [Qemu-devel] [PATCH qom-next] scsi: Improve error propagation for scsi_bus_legacy_handle_cmdline()

[Hu Tao]

On Sun, Jul 21, 2013 at 12:30:54PM +0200, Andreas Färber wrote:
> Let scsi_bus_legacy_add_drive() and scsi_bus_legacy_handle_cmdline()
> return an Error**. Prepare qdev initfns for QOM realize error model.
> 
> Signed-off-by: Andreas Färber <address@hidden>
> ---
>  hw/pci/pci-hotplug-old.c |  2 +-
>  hw/scsi/esp-pci.c        |  7 ++++++-
>  hw/scsi/esp.c            |  6 ++++--
>  hw/scsi/lsi53c895a.c     |  7 ++++++-
>  hw/scsi/megasas.c        |  7 ++++++-
>  hw/scsi/scsi-bus.c       | 22 +++++++++++++++-------
>  hw/scsi/spapr_vscsi.c    |  7 ++++++-
>  hw/scsi/virtio-scsi.c    |  7 ++++++-
>  hw/usb/dev-storage.c     |  4 +++-
>  include/hw/scsi/scsi.h   |  4 ++--
>  10 files changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/pci/pci-hotplug-old.c b/hw/pci/pci-hotplug-old.c
> index 8077289..619fe47 100644
> --- a/hw/pci/pci-hotplug-old.c
> +++ b/hw/pci/pci-hotplug-old.c
> @@ -127,7 +127,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState 
> *adapter,
>      dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1);
>      dinfo->bus = scsibus->busnr;
>      scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit,
> -                                        false, -1, NULL);
> +                                        false, -1, NULL, NULL);
>      if (!scsidev) {
>          return -1;
>      }
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index 6cdfd53..2ac21d4 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -346,6 +346,7 @@ static int esp_pci_scsi_init(PCIDevice *dev)
>      DeviceState *d = DEVICE(dev);
>      ESPState *s = &pci->esp;
>      uint8_t *pci_conf;
> +    Error *err = NULL;
>  
>      pci_conf = dev->config;
>  
> @@ -364,7 +365,11 @@ static int esp_pci_scsi_init(PCIDevice *dev)
>  
>      scsi_bus_new(&s->bus, d, &esp_pci_scsi_info, NULL);
>      if (!d->hotplugged) {
> -        return scsi_bus_legacy_handle_cmdline(&s->bus);
> +        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
> +        if (err != NULL) {

           if (error_is_set(&err))

> +            error_free(err);
> +            return -1;
> +        }
>      }
>      return 0;
>  }
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index c24b632..94639b8 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -676,6 +676,7 @@ static void sysbus_esp_realize(DeviceState *dev, Error 
> **errp)
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      SysBusESPState *sysbus = ESP(dev);
>      ESPState *s = &sysbus->esp;
> +    Error *err = NULL;
>  
>      sysbus_init_irq(sbd, &s->irq);
>      assert(sysbus->it_shift != -1);
> @@ -688,8 +689,9 @@ static void sysbus_esp_realize(DeviceState *dev, Error 
> **errp)
>      qdev_init_gpio_in(dev, sysbus_esp_gpio_demux, 2);
>  
>      scsi_bus_new(&s->bus, dev, &esp_scsi_info, NULL);
> -    if (scsi_bus_legacy_handle_cmdline(&s->bus) < 0) {
> -        error_setg(errp, "Handling legacy SCSI command line failed");
> +    scsi_bus_legacy_handle_cmdline(&s->bus, &err);
> +    if (err != NULL) {

       if (error_is_set(&err))

> +        error_propagate(errp, err);
>          return;
>      }
>  }
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index e11224f..776e31a 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -2096,6 +2096,7 @@ static int lsi_scsi_init(PCIDevice *dev)
>      LSIState *s = LSI53C895A(dev);
>      DeviceState *d = DEVICE(dev);
>      uint8_t *pci_conf;
> +    Error *err = NULL;
>  
>      pci_conf = dev->config;
>  
> @@ -2118,7 +2119,11 @@ static int lsi_scsi_init(PCIDevice *dev)
>  
>      scsi_bus_new(&s->bus, d, &lsi_scsi_info, NULL);
>      if (!d->hotplugged) {
> -        return scsi_bus_legacy_handle_cmdline(&s->bus);
> +        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
> +        if (err != NULL) {

           if (error_is_set(&err))

> +            error_free(err);
> +            return -1;
> +        }
>      }
>      return 0;
>  }
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 2ae4346..eb52164 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2102,6 +2102,7 @@ static int megasas_scsi_init(PCIDevice *dev)
>      MegasasState *s = MEGASAS(dev);
>      uint8_t *pci_conf;
>      int i, bar_type;
> +    Error *err = NULL;
>  
>      pci_conf = dev->config;
>  
> @@ -2172,7 +2173,11 @@ static int megasas_scsi_init(PCIDevice *dev)
>  
>      scsi_bus_new(&s->bus, DEVICE(dev), &megasas_scsi_info, NULL);
>      if (!d->hotplugged) {
> -        return scsi_bus_legacy_handle_cmdline(&s->bus);
> +        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
> +        if (err != NULL) {

           if (error_is_set(&err))

> +            error_free(err);
> +            return -1;
> +        }
>      }
>      return 0;
>  }
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index a92b7c1..b5a863a 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -209,10 +209,11 @@ static int scsi_qdev_exit(DeviceState *qdev)
>  /* handle legacy '-drive if=scsi,...' cmd line args */
>  SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
>                                        int unit, bool removable, int 
> bootindex,
> -                                      const char *serial)
> +                                      const char *serial, Error **errp)
>  {
>      const char *driver;
>      DeviceState *dev;
> +    Error *err = NULL;
>  
>      driver = bdrv_is_sg(bdrv) ? "scsi-generic" : "scsi-disk";
>      dev = qdev_create(&bus->qbus, driver);
> @@ -227,19 +228,25 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, 
> BlockDriverState *bdrv,
>          qdev_prop_set_string(dev, "serial", serial);
>      }
>      if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) {
> +        error_setg(errp, "Setting drive property failed");
>          qdev_free(dev);
>          return NULL;
>      }
> -    if (qdev_init(dev) < 0)
> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> +    if (err != NULL) {

       if (error_is_set(&err))

> +        error_propagate(errp, err);
> +        qdev_free(dev);
>          return NULL;
> +    }
>      return SCSI_DEVICE(dev);
>  }
>  
> -int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
> +void scsi_bus_legacy_handle_cmdline(SCSIBus *bus, Error **errp)
>  {
>      Location loc;
>      DriveInfo *dinfo;
> -    int res = 0, unit;
> +    int unit;
> +    Error *err = NULL;
>  
>      loc_push_none(&loc);
>      for (unit = 0; unit <= bus->info->max_target; unit++) {
> @@ -248,13 +255,14 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
>              continue;
>          }
>          qemu_opts_loc_restore(dinfo->opts);
> -        if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit, false, -1, 
> NULL)) {
> -            res = -1;
> +        scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit, false, -1, NULL,
> +                                  &err);
> +        if (err != NULL) {

           if (error_is_set(&err))

> +            error_propagate(errp, err);
>              break;
>          }
>      }
>      loc_pop(&loc);
> -    return res;
>  }
>  
>  static int32_t scsi_invalid_field(SCSIRequest *req, uint8_t *buf)
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index e8978bf..55b44b9 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -912,12 +912,17 @@ static void spapr_vscsi_reset(VIOsPAPRDevice *dev)
>  static int spapr_vscsi_init(VIOsPAPRDevice *dev)
>  {
>      VSCSIState *s = VIO_SPAPR_VSCSI_DEVICE(dev);
> +    Error *err = NULL;
>  
>      dev->crq.SendFunc = vscsi_do_crq;
>  
>      scsi_bus_new(&s->bus, &dev->qdev, &vscsi_scsi_info, NULL);
>      if (!dev->qdev.hotplugged) {
> -        scsi_bus_legacy_handle_cmdline(&s->bus);
> +        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
> +        if (err != NULL) {

           if (error_is_set(&err))

> +            error_free(err);
> +            return -1;
> +        }
>      }
>  
>      return 0;
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 712f0ad..42cb73b 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -619,6 +619,7 @@ static int virtio_scsi_device_init(VirtIODevice *vdev)
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
>      VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>      static int virtio_scsi_id;
> +    Error *err = NULL;
>      int ret;
>  
>      ret = virtio_scsi_common_init(vs);
> @@ -629,7 +630,11 @@ static int virtio_scsi_device_init(VirtIODevice *vdev)
>      scsi_bus_new(&s->bus, qdev, &virtio_scsi_scsi_info, vdev->bus_name);
>  
>      if (!qdev->hotplugged) {
> -        scsi_bus_legacy_handle_cmdline(&s->bus);
> +        scsi_bus_legacy_handle_cmdline(&s->bus, &err);
> +        if (err != NULL) {

           if (error_is_set(&err))

> +            error_free(err);
> +            return -1;
> +        }
>      }
>  
>      register_savevm(qdev, "virtio-scsi", virtio_scsi_id++, 1,
> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
> index fe914ab..1954811 100644
> --- a/hw/usb/dev-storage.c
> +++ b/hw/usb/dev-storage.c
> @@ -595,6 +595,7 @@ static int usb_msd_initfn_storage(USBDevice *dev)
>      MSDState *s = DO_UPCAST(MSDState, dev, dev);
>      BlockDriverState *bs = s->conf.bs;
>      SCSIDevice *scsi_dev;
> +    Error *err = NULL;
>  
>      if (!bs) {
>          error_report("drive property not set");
> @@ -619,7 +620,8 @@ static int usb_msd_initfn_storage(USBDevice *dev)
>      usb_desc_init(dev);
>      scsi_bus_new(&s->bus, &s->dev.qdev, &usb_msd_scsi_info_storage, NULL);
>      scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0, !!s->removable,
> -                                            s->conf.bootindex, dev->serial);
> +                                         s->conf.bootindex, dev->serial,
> +                                         &err);
>      if (!scsi_dev) {
>          return -1;
>      }
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 9786e00..8786531 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -162,8 +162,8 @@ static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
>  
>  SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv,
>                                        int unit, bool removable, int 
> bootindex,
> -                                      const char *serial);
> -int scsi_bus_legacy_handle_cmdline(SCSIBus *bus);
> +                                      const char *serial, Error **errp);
> +void scsi_bus_legacy_handle_cmdline(SCSIBus *bus, Error **errp);
>  
>  /*
>   * Predefined sense codes
> -- 
> 1.8.1.4
> 
>