Re: [PATCH 01/10] qdev: add "check if address free" callback for buses

[Maxim Levitsky]

On Fri, 2020-09-25 at 13:25 -0400, Paolo Bonzini wrote:
> Check if an address is free on the bus before plugging in the
> device.  This makes it possible to do the check without any
> side effects, and to detect the problem early without having
> to do it in the realize callback.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/core/qdev.c         | 17 +++++++++++++++--
>  hw/net/virtio-net.c    |  2 +-
>  hw/sd/core.c           |  3 ++-
>  include/hw/qdev-core.h |  3 ++-
>  4 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 96772a15bd..74db78df36 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -94,13 +94,23 @@ static void bus_add_child(BusState *bus, DeviceState 
> *child)
>                               0);
>  }
>  
> -void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
> +static bool bus_check_address(BusState *bus, DeviceState *child, Error 
> **errp)
> +{
> +    BusClass *bc = BUS_GET_CLASS(bus);
> +    return !bc->check_address || bc->check_address(bus, child, errp);
> +}
> +
> +bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
>  {
>      BusState *old_parent_bus = dev->parent_bus;
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>  
>      assert(dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type));
>  
> +    if (!bus_check_address(bus, dev, errp)) {
> +        return false;
> +    }
> +
>      if (old_parent_bus) {
>          trace_qdev_update_parent_bus(dev, object_get_typename(OBJECT(dev)),
>              old_parent_bus, object_get_typename(OBJECT(old_parent_bus)),
> @@ -126,6 +136,7 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>          object_unref(OBJECT(old_parent_bus));
>          object_unref(OBJECT(dev));
>      }
> +    return true;
>  }
>  
>  DeviceState *qdev_new(const char *name)
> @@ -371,7 +382,9 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error 
> **errp)
>      assert(!dev->realized && !dev->parent_bus);
>  
>      if (bus) {
> -        qdev_set_parent_bus(dev, bus);
> +        if (!qdev_set_parent_bus(dev, bus, errp)) {
> +            return false;
> +        }
>      } else {
>          assert(!DEVICE_GET_CLASS(dev)->bus_type);
>      }
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7bf27b9db7..268cecc498 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3142,7 +3142,7 @@ static bool failover_replug_primary(VirtIONet *n, Error 
> **errp)
>          error_setg(errp, "virtio_net: couldn't find primary bus");
>          return false;
>      }
> -    qdev_set_parent_bus(n->primary_dev, n->primary_bus);
> +    qdev_set_parent_bus(n->primary_dev, n->primary_bus, &error_abort);
>      n->primary_should_be_hidden = false;
>      if (!qemu_opt_set_bool(n->primary_device_opts,
>                             "partially_hotplugged", true, errp)) {
> diff --git a/hw/sd/core.c b/hw/sd/core.c
> index 957d116f1a..08c93b5903 100644
> --- a/hw/sd/core.c
> +++ b/hw/sd/core.c
> @@ -23,6 +23,7 @@
>  #include "hw/qdev-core.h"
>  #include "hw/sd/sd.h"
>  #include "qemu/module.h"
> +#include "qapi/error.h"
>  #include "trace.h"
>  
>  static inline const char *sdbus_name(SDBus *sdbus)
> @@ -240,7 +241,7 @@ void sdbus_reparent_card(SDBus *from, SDBus *to)
>      readonly = sc->get_readonly(card);
>  
>      sdbus_set_inserted(from, false);
> -    qdev_set_parent_bus(DEVICE(card), &to->qbus);
> +    qdev_set_parent_bus(DEVICE(card), &to->qbus, &error_abort);
>      sdbus_set_inserted(to, true);
>      sdbus_set_readonly(to, readonly);
>  }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 72064f4dd4..e62da68a26 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -217,6 +217,7 @@ struct BusClass {
>       */
>      char *(*get_fw_dev_path)(DeviceState *dev);
>      void (*reset)(BusState *bus);
> +    bool (*check_address)(BusState *bus, DeviceState *dev, Error **errp);
>      BusRealize realize;
>      BusUnrealize unrealize;
>  
> @@ -788,7 +789,7 @@ const char *qdev_fw_name(DeviceState *dev);
>  Object *qdev_get_machine(void);
>  
>  /* FIXME: make this a link<> */
> -void qdev_set_parent_bus(DeviceState *dev, BusState *bus);
> +bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
>  
>  extern bool qdev_hotplug;
>  extern bool qdev_hot_removed;


I like that idea, however I wonder why this was needed.
My patch that switches the direction in scsi_device_find, is supposed to be 
completely equavalent, 
based on the following train of thought:

If scsi_device_find finds an exact match it returns only it, as before.

Otherwise scsi_device_find were to scan from end of the list to the start, and 
every time,
it finds a device with same channel/id it would update the target_dev
and return it when it reaches the end of the list. 

If I am not mistaken this means that it would return _first_ device in the 
list that matches the channel/id.
This is exactly what new version of scsi_device_find does.

Anyway, since I don't see anything wrong with doing what this patch does other
than adding a documentation to the function as Stefan pointed out,

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
        Maxim Levitsky