On 22.08.2017 17:30, Peter Maydell wrote: > On 22 August 2017 at 16:26, Thomas Huth <th...@redhat.com> wrote: >> On 22.08.2017 17:12, Peter Maydell wrote: >>> Why is our default "hotpluggable" rather than "not hotpluggable" ? >>> We must have way more non-hotpluggable devices than hotpluggable >>> ones, and it takes active effort to make a hotpluggable device >>> model, so it seems like it would be much less bug-prone to >>> require hotpluggable devices to set dc->hotpluggable true >>> rather than all the non-hotpluggable ones to set it false... >> >> I think most devices are already non-hotpluggable automatically because >> they sit on a bus that is not hot-pluggable (e.g. sysbus devices). The >> problematic ones are the devices with .parent = TYPE_DEVICE. And for >> these, I think it is quite hard to say whether they should be >> hot-pluggable by default or not? > > To implement hotplug you need to write extra code (notably some > kind of unrealize method to undo whatever you did in realize), > at which point also setting the hotpluggable flag is trivial. > >> Anyway, according to my tests (I'm currently working on a test that >> automatically does device_add + device_del for all devices, as you might >> have guessed already), there are not that many devices that cause >> problems here, so I guess marking some few with hotpluggable = false >> should be OK? > > The problem is not the devices we have today but the ones we're > going to write tomorrow. Having hotpluggable default to false > "fails safe" -- the worst that happens is that somebody writing > a hotpluggable device finds in their testing that they need to > set the flag to make it work. Having it default to true fails > non-safely -- as you've found, people writing devices that > were never expected to be hotplugged don't try to test the > error case of attempting hotplug anyway, and in code review > the absence of a line of code is very hard to reliably spot, > so we get devices in tree that crash QEMU if you try to > hotplug them. If we make the default be not-hotpluggable > we fix this not just today but forever.
OK, you've got a point. I'll ponder about this a little bit and will try to come up with a patch / some patches ... Thomas