Andreas Färber <afaer...@suse.de> writes: > Am 01.12.2013 14:13, schrieb Marcel Apfelbaum: >> On Fri, 2013-11-29 at 10:43 +0100, arm...@redhat.com wrote: >>> From: Markus Armbruster <arm...@redhat.com> >>> >>> Pointer properties can be set only by code, not by device_add. A >>> device with a pointer property can't work with device_add only unless >>> the property may remain null. cannot_instantiate_with_device_add_yet >>> needs to be set then. PATCH 1/2 sets it when needed and else >>> documents why not. PATCH 2/2 documents this for future users of >>> pointer properties. >>> >>> This applies on top of my "[PATCH v4 00/10] Clean up and fix no_user" >>> series. >> >> Even that I am not familiar with this code, I've checked all the changes >> and I agree with them. >> >> Reviewed-by: Marcel Apfelbaum <marce...@redhat.com> >> >> Anyway, I do have a question: >> Why not asserting on qdev_device_add if we have a pointer property?
This is a really good thought. In fact, it occurred to me, too. However, see "unless the property may remain null" above: there are uses of pointer properties that do *not* make the device unusable with device_add. We even have an example: etraxfs,pic; see PATCH 1/1. It's a sysbus device, so it's unavailable anyway. But there certainly could be a device with an optional property that does not and should not have cannot_instantiate_with_device_add_yet set. > When we do device_add / device-add, the guest is usually running and we > shouldn't kill a running guest just because the user is trying something > stupid that we can easily prevent. ;) You have a point on assert(bad_input), but this would be assert(programming_error), where the error is "device doesn't have cannot_instantiate_with_device_add_yet set". I'm advocating to be ruthless with programming error asserts. > The alternative BTW is dropping all those pointer properties and > replacing them with link<> properties. Paolo tried that for the OMAP > timers once but I fear that series was never picked up...? /* FIXME: Remove opaque pointer properties. */ /* Not a proper property, just for dirty hacks. TODO Remove it! */ :) >> Instead of checking only cannot_instantiate_with_device_add_yet, >> we can go over properties and if we have a pointer property, assert or >> return... > > Raising an error for certain property types may be an option. Although > theoretically the existence of an incompatible property would not > necessarily indicate incompatibility to instantiate the device, in > practice I believe we don't have such excess properties. We don't have them now. I hope we won't permit any new pointer properties. If you guys want pointer property imply its owner's cannot_instantiate_with_device_add_yet, even though it's not generally necessary, I'm fine with that. [...]
