On Mon, Jul 16, 2018 at 09:09:13AM +0200, Thomas Huth wrote:
> On 13.07.2018 23:26, Eduardo Habkost wrote:
> > On Fri, Jul 13, 2018 at 10:27:31AM +0200, Thomas Huth wrote:
> >> When trying to "device_add bcm2837" on a machine that is not suitable for
> >> this device, you can quickly crash QEMU afterwards, e.g. with "info qtree":
> >>
> >> echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
> >> "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', "
> >> \
> >> "'arguments': {'command-line': 'info qtree'}}" | \
> >> aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp
> >> stdio
> >>
> >> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
> >> "package": "build-all"}, "capabilities": []}}
> >> {"return": {}}
> >> {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
> >> hotplugged on this machine"}}
> >> Segmentation fault (core dumped)
> >>
> >> The qdev_set_parent_bus() from instance_init adds a link to the child
> >> devices
> >> which is not valid anymore after the bcm2837 instance has been destroyed.
> >> Unfortunately, the child devices do not get destroyed / unlinked correctly
> >> because both object_initialize() and object_property_add_child() increase
> >> the reference count of the child objects by one, but only one reference
> >> is dropped when the parent gets removed. So let's use the new functions
> >> object_initialize_child() and sysbus_init_child_obj() instead to create
> >> the objects, which will take care of creating the child objects with the
> >> correct reference count of one.
> >>
> >> Signed-off-by: Thomas Huth <th...@redhat.com>
> >
> > Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
> >
> > The usage of &error_abort in code that can be triggered from
> > device-list-properties still makes me nervous, but that's a
> > separate issue.
>
> I first had similar thoughts, but I think it's a clear coding issue if
> the abort triggers here (and not something that the user should normally
> be able to trigger somehow), so error_abort should be ok in this case.
Agreed. We don't really have an option, as instance_init can't
report errors.
--
Eduardo
