Re: [Qemu-devel] Hot-pluggable device without ->unrealize() is highly suspect (was: [PATCH] hw: scsi: dc390: add device unrealize function)

[Peter Maydell]

On Thu, 29 Nov 2018 at 19:03, Markus Armbruster <address@hidden> wrote:
>
> Li Qiang <address@hidden> writes:
>
> > Currently the dc390 device has no unrealize function. This
> > can cause memory leak when hotplug/unplug device. Also more
> > serious, it will trigger an assert when rehotplug.
> [...]
>
> When a hot-pluggable device doesn't have an ->unrealize() method, unplug
> is probably broken.  I think we should track down such devices for
> review.  Any takers?

Add an assert somewhere and catch it with the usual
"instantiate everything" qtest?

More generally, what is causing dc390 to be hotpluggable?
I can't see anything obvious in the class init. If we
have APIs where the default is "you get this weird thing
you weren't thinking about but it's broken because
you weren't thinking about it" then we will have a whole
class of bugs that we then need to squash device by device[*].
I think it would be better for devices to have to explicitly
opt in to implementing things like hotplug -- that way the
failure mode is just "this isn't hotpluggable", we can
report that helpfully to the user, and if anybody cares
they can add the support.

[*] Also currently true for migration support. We should
require devices to either provide a VMState or explicitly
say they have no state needing migration or explicitly
say they don't support migration, and then assert that
they do one or the other of those, rather than having the
default be "we'll allow migration but not migrate any
of the device's state".

thanks
-- PMM