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

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

[Peter Maydell]

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.

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).)
+         */
+        g_assert(object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE) != NULL);
         bus = sysbus_get_default();
     }
 
-- 
1.9.1

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

[Markus Armbruster]

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>

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

[Peter Maydell]

On 11 May 2016 at 08:24, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>>  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>

Thanks, I have fixed the minor nits and applied this to master.

-- PMM