Peter Maydell <peter.mayd...@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.mayd...@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 <arm...@redhat.com>
