| Message ID | 1458137462-30587-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
Am 16.03.2016 um 15:11 schrieb 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> > --- > This needs to go in after http://patchwork.ozlabs.org/patch/597716/ > as otherwise the bug fixed by that patch will become a 'make check' > failure. Looks strange, but okay, Reviewed-by: Andreas Färber <afaerber@suse.de> Through whose queue? Thanks, Andreas
On 16 March 2016 at 14:14, Andreas Färber <afaerber@suse.de> wrote: > Am 16.03.2016 um 15:11 schrieb 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> >> --- >> This needs to go in after http://patchwork.ozlabs.org/patch/597716/ >> as otherwise the bug fixed by that patch will become a 'make check' >> failure. > > Looks strange, but okay, You mean the way we use what looks like a cast macro and ignore the result? Yeah, I thought that was a little odd-looking. Happy to do it some other way if you have an alternative suggestion. > Reviewed-by: Andreas Färber <afaerber@suse.de> > > Through whose queue? I'm happy to take it via the target-arm queue, since I was planning to take the sd.c fix that way and this one needs to go after it, if that works for you. thanks -- PMM
On 16/03/2016 15:25, Peter Maydell wrote: > > Looks strange, but okay, > You mean the way we use what looks like a cast macro and ignore > the result? Yeah, I thought that was a little odd-looking. Happy > to do it some other way if you have an alternative suggestion. Perhaps this: g_assert(object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE) != NULL); or just add a (void) in front of the cast. Paolo
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index db41aa1..fb7db86 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).) + */ + SYS_BUS_DEVICE(dev); bus = sysbus_get_default(); }
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> --- This needs to go in after http://patchwork.ozlabs.org/patch/597716/ as otherwise the bug fixed by that patch will become a 'make check' failure. hw/core/qdev.c | 7 +++++++ 1 file changed, 7 insertions(+)