Re: [Qemu-devel] [PATCH v2] qdev_try_create(): Assert that devices we put onto the system bus are SysBusDevices

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> If qdev_try_create() is passed NULL for the bus, it will automatically
> put the newly created device onto the default system bus. However
> if the device is not actually a SysBusDevice then this will result
> in later crashes (for instance when running the monitor "info qtree"
> command) because code reasonably assumes that all devices on the system
> bus are system bus devices.
>
> Generally the mistake is that the calling code should create the
> object with object_new(TYPE_FOO) rather than qdev_create(NULL, TYPE_FOO);
> see commit 6749695eaaf346c1 for an example of fixing this bug.
>
> Assert in qdev_try_create() if the device isn't suitable to put on
> the system bus, so that this mistake results in failure earlier
> and more reliably.

Asserting sooner rather than later makes sense.

I consider "NULL means default system bus" a design wart.  Can we get
rid of it?  How much do we rely on this wart?

These questions are no reason to delay this patch.

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> v1->v2: use an explicit g_assert() rather than relying on the one
> hidden inside a SYS_BUS_DEVICE(dev) cast macro, as suggested by Paolo.
>
> Andreas: the SD card patch which had to go in before this patch is
> already in master, so you can just take this patch via your QOM tree
> without it causing any awkward ordering issues.
>
>  hw/core/qdev.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index db41aa1..15b6713 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -35,6 +35,7 @@
>  #include "qemu/error-report.h"
>  #include "hw/hotplug.h"
>  #include "hw/boards.h"
> +#include "hw/sysbus.h"
>  #include "qapi-event.h"
>  
>  int qdev_hotplug = 0;
> @@ -161,6 +162,12 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
>      }
>  
>      if (!bus) {
> +        /* Assert that the device really is a SysBusDevice before
> +         * we put it onto the sysbus. (Non-sysbus devices which aren't
> +         * being put onto a bus should be created with object_new(TYPE_FOO),
> +         * not qdev_create(NULL, TYPE_FOO).)

Parenthesizing a sentence like this looks odd to me.  I'd drop the
parens.

> +         */
> +        g_assert(object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE) != NULL);

Long line, easy to fix: drop the redundant != NULL.

>          bus = sysbus_get_default();
>      }

Just nits, no respin required.  Perhaps they can be addressed on commit.

Reviewed-by: Markus Armbruster <armbru@redhat.com>