Re: [PATCH 1/6] scsi: Replace scsi_bus_new() with scsi_bus_init(), scsi_bus_init_named()

[Paolo Bonzini <pbonzini@redhat.com>]

On 23/09/21 14:11, Peter Maydell wrote:
> The function scsi_bus_new() creates a new SCSI bus; callers can
> either pass in a name argument to specify the name of the new bus, or
> they can pass in NULL to allow the bus to be given an automatically
> generated unique name.  Almost all callers want to use the
> autogenerated name; the only exception is the virtio-scsi device.
> 
> Taking a name argument that should almost always be NULL is an
> easy-to-misuse API design -- it encourages callers to think perhaps
> they should pass in some standard name like "scsi" or "scsi-bus".  We
> don't do this anywhere for SCSI, but we do (incorrectly) do it for
> other bus types such as i2c.
> 
> The function name also implies that it will return a newly allocated
> object, when it in fact does in-place allocation.  We more commonly
> name such functions foo_init(), with foo_new() being the
> allocate-and-return variant.
> 
> Replace all the scsi_bus_new() callsites with either:
>   * scsi_bus_init() for the usual case where the caller wants
>     an autogenerated bus name
>   * scsi_bus_init_named() for the rare case where the caller
>     needs to specify the bus name
> 
> and document that for the _named() version it's then the caller's
> responsibility to think about uniqueness of bus names.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/scsi/scsi.h       | 30 ++++++++++++++++++++++++++++--
>   hw/scsi/esp-pci.c            |  2 +-
>   hw/scsi/esp.c                |  2 +-
>   hw/scsi/lsi53c895a.c         |  2 +-
>   hw/scsi/megasas.c            |  3 +--
>   hw/scsi/mptsas.c             |  2 +-
>   hw/scsi/scsi-bus.c           |  4 ++--
>   hw/scsi/spapr_vscsi.c        |  3 +--
>   hw/scsi/virtio-scsi.c        |  4 ++--
>   hw/scsi/vmw_pvscsi.c         |  3 +--
>   hw/usb/dev-storage-bot.c     |  3 +--
>   hw/usb/dev-storage-classic.c |  4 ++--
>   hw/usb/dev-uas.c             |  3 +--
>   13 files changed, 43 insertions(+), 22 deletions(-)
> 
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 0b726bc78c6..a567a5ed86b 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -146,8 +146,34 @@ struct SCSIBus {
>       const SCSIBusInfo *info;
>   };
>   
> -void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
> -                  const SCSIBusInfo *info, const char *bus_name);
> +/**
> + * scsi_bus_init_named: Initialize a SCSI bus with the specified name
> + * @bus: SCSIBus object to initialize
> + * @bus_size: size of @bus object
> + * @host: Device which owns the bus (generally the SCSI controller)
> + * @info: structure defining callbacks etc for the controller
> + * @bus_name: Name to use for this bus
> + *
> + * This in-place initializes @bus as a new SCSI bus with a name
> + * provided by the caller. It is the caller's responsibility to make
> + * sure that name does not clash with the name of any other bus in the
> + * system. Unless you need the new bus to have a specific name, you
> + * should use scsi_bus_new() instead.
> + */
> +void scsi_bus_init_named(SCSIBus *bus, size_t bus_size, DeviceState *host,
> +                         const SCSIBusInfo *info, const char *bus_name);
> +
> +/**
> + * scsi_bus_init: Initialize a SCSI bus
> + *
> + * This in-place-initializes @bus as a new SCSI bus and gives it
> + * an automatically generated unique name.
> + */
> +static inline void scsi_bus_init(SCSIBus *bus, size_t bus_size,
> +                                 DeviceState *host, const SCSIBusInfo *info)
> +{
> +    scsi_bus_init_named(bus, bus_size, host, info, NULL);
> +}
>   
>   static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d)
>   {
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index 9db10b1a487..dac054aeed4 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -388,7 +388,7 @@ static void esp_pci_scsi_realize(PCIDevice *dev, Error **errp)
>       pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &pci->io);
>       s->irq = pci_allocate_irq(dev);
>   
> -    scsi_bus_new(&s->bus, sizeof(s->bus), d, &esp_pci_scsi_info, NULL);
> +    scsi_bus_init(&s->bus, sizeof(s->bus), d, &esp_pci_scsi_info);
>   }
>   
>   static void esp_pci_scsi_exit(PCIDevice *d)
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 4ac21147888..8454ed17735 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -1348,7 +1348,7 @@ static void sysbus_esp_realize(DeviceState *dev, Error **errp)
>   
>       qdev_init_gpio_in(dev, sysbus_esp_gpio_demux, 2);
>   
> -    scsi_bus_new(&s->bus, sizeof(s->bus), dev, &esp_scsi_info, NULL);
> +    scsi_bus_init(&s->bus, sizeof(s->bus), dev, &esp_scsi_info);
>   }
>   
>   static void sysbus_esp_hard_reset(DeviceState *dev)
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index e2c19180a0d..85e907a7854 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -2309,7 +2309,7 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
>       pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->ram_io);
>       QTAILQ_INIT(&s->queue);
>   
> -    scsi_bus_new(&s->bus, sizeof(s->bus), d, &lsi_scsi_info, NULL);
> +    scsi_bus_init(&s->bus, sizeof(s->bus), d, &lsi_scsi_info);
>   }
>   
>   static void lsi_scsi_exit(PCIDevice *dev)
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 8f2389d2c6a..4ff51221d4c 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2416,8 +2416,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>           s->frames[i].state = s;
>       }
>   
> -    scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
> -                 &megasas_scsi_info, NULL);
> +    scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(dev), &megasas_scsi_info);
>   }
>   
>   static Property megasas_properties_gen1[] = {
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index db3219e7d20..f6c77655443 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1315,7 +1315,7 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
>   
>       s->request_bh = qemu_bh_new(mptsas_fetch_requests, s);
>   
> -    scsi_bus_new(&s->bus, sizeof(s->bus), &dev->qdev, &mptsas_scsi_info, NULL);
> +    scsi_bus_init(&s->bus, sizeof(s->bus), &dev->qdev, &mptsas_scsi_info);
>   }
>   
>   static void mptsas_scsi_uninit(PCIDevice *dev)
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 2a0a98cac91..e28a6ea053a 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -134,8 +134,8 @@ void scsi_device_unit_attention_reported(SCSIDevice *s)
>   }
>   
>   /* Create a scsi bus, and attach devices to it.  */
> -void scsi_bus_new(SCSIBus *bus, size_t bus_size, DeviceState *host,
> -                  const SCSIBusInfo *info, const char *bus_name)
> +void scsi_bus_init_named(SCSIBus *bus, size_t bus_size, DeviceState *host,
> +                         const SCSIBusInfo *info, const char *bus_name)
>   {
>       qbus_create_inplace(bus, bus_size, TYPE_SCSI_BUS, host, bus_name);
>       bus->busnr = next_scsi_bus++;
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index c210262484a..a07a8e1523f 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -1216,8 +1216,7 @@ static void spapr_vscsi_realize(SpaprVioDevice *dev, Error **errp)
>   
>       dev->crq.SendFunc = vscsi_do_crq;
>   
> -    scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
> -                 &vscsi_scsi_info, NULL);
> +    scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(dev), &vscsi_scsi_info);
>   
>       /* ibmvscsi SCSI bus does not allow hotplug. */
>       qbus_set_hotplug_handler(BUS(&s->bus), NULL);
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 6d807302870..51fd09522ac 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -1019,8 +1019,8 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>   
> -    scsi_bus_new(&s->bus, sizeof(s->bus), dev,
> -                 &virtio_scsi_scsi_info, vdev->bus_name);
> +    scsi_bus_init_named(&s->bus, sizeof(s->bus), dev,
> +                       &virtio_scsi_scsi_info, vdev->bus_name);
>       /* override default SCSI bus hotplug-handler, with virtio-scsi's one */
>       qbus_set_hotplug_handler(BUS(&s->bus), OBJECT(dev));
>   
> diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> index 1f30cb020a1..cd76bd67ab7 100644
> --- a/hw/scsi/vmw_pvscsi.c
> +++ b/hw/scsi/vmw_pvscsi.c
> @@ -1180,8 +1180,7 @@ pvscsi_realizefn(PCIDevice *pci_dev, Error **errp)
>   
>       s->completion_worker = qemu_bh_new(pvscsi_process_completion_queue, s);
>   
> -    scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(pci_dev),
> -                 &pvscsi_scsi_info, NULL);
> +    scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(pci_dev), &pvscsi_scsi_info);
>       /* override default SCSI bus hotplug-handler, with pvscsi's one */
>       qbus_set_hotplug_handler(BUS(&s->bus), OBJECT(s));
>       pvscsi_reset_state(s);
> diff --git a/hw/usb/dev-storage-bot.c b/hw/usb/dev-storage-bot.c
> index 68ebaca10c6..b24b3148c28 100644
> --- a/hw/usb/dev-storage-bot.c
> +++ b/hw/usb/dev-storage-bot.c
> @@ -37,8 +37,7 @@ static void usb_msd_bot_realize(USBDevice *dev, Error **errp)
>           s->dev.auto_attach = 0;
>       }
>   
> -    scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
> -                 &usb_msd_scsi_info_bot, NULL);
> +    scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(dev), &usb_msd_scsi_info_bot);
>       usb_msd_handle_reset(dev);
>   }
>   
> diff --git a/hw/usb/dev-storage-classic.c b/hw/usb/dev-storage-classic.c
> index 3d017a4e679..00f25bade28 100644
> --- a/hw/usb/dev-storage-classic.c
> +++ b/hw/usb/dev-storage-classic.c
> @@ -65,8 +65,8 @@ static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
>       usb_desc_create_serial(dev);
>       usb_desc_init(dev);
>       dev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
> -    scsi_bus_new(&s->bus, sizeof(s->bus), DEVICE(dev),
> -                 &usb_msd_scsi_info_storage, NULL);
> +    scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(dev),
> +                 &usb_msd_scsi_info_storage);
>       scsi_dev = scsi_bus_legacy_add_drive(&s->bus, blk, 0, !!s->removable,
>                                            s->conf.bootindex, s->conf.share_rw,
>                                            s->conf.rerror, s->conf.werror,
> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
> index f6309a5ebfd..599d6b52a01 100644
> --- a/hw/usb/dev-uas.c
> +++ b/hw/usb/dev-uas.c
> @@ -938,8 +938,7 @@ static void usb_uas_realize(USBDevice *dev, Error **errp)
>       uas->status_bh = qemu_bh_new(usb_uas_send_status_bh, uas);
>   
>       dev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
> -    scsi_bus_new(&uas->bus, sizeof(uas->bus), DEVICE(dev),
> -                 &usb_uas_scsi_info, NULL);
> +    scsi_bus_init(&uas->bus, sizeof(uas->bus), DEVICE(dev), &usb_uas_scsi_info);
>   }
>   
>   static const VMStateDescription vmstate_usb_uas = {
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks,

Paolo