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 <pbonz...@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 <mlevi...@redhat.com>
Best regards,
Maxim Levitsky
