[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] qdev_try_create(): Assert that devices we pu

From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v2] qdev_try_create(): Assert that devices we put onto the system bus are SysBusDevices
Date: Wed, 11 May 2016 09:24:56 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Peter Maydell <address@hidden> 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 <address@hidden>
> ---
> 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

> +         */
> +        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 <address@hidden>

reply via email to

[Prev in Thread] Current Thread [Next in Thread]