From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57069) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0OVz-0006pl-LE for qemu-devel@nongnu.org; Wed, 11 May 2016 03:25:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b0OVw-0006rn-9t for qemu-devel@nongnu.org; Wed, 11 May 2016 03:25:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55935) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0OVw-0006rf-0p for qemu-devel@nongnu.org; Wed, 11 May 2016 03:25:00 -0400 From: Markus Armbruster References: <1462876242-16009-1-git-send-email-peter.maydell@linaro.org> Date: Wed, 11 May 2016 09:24:56 +0200 In-Reply-To: <1462876242-16009-1-git-send-email-peter.maydell@linaro.org> (Peter Maydell's message of "Tue, 10 May 2016 11:30:42 +0100") Message-ID: <877ff1jbuf.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2] qdev_try_create(): Assert that devices we put onto the system bus are SysBusDevices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org, Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= , patches@linaro.org Peter Maydell 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 > --- > 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