Re: [Qemu-devel] [PATCH 0/2] Pointer properties and device_add

[Marcel Apfelbaum]

On Mon, 2013-12-02 at 08:30 +0100, Markus Armbruster wrote:
> Andreas Färber <address@hidden> writes:
> 
> > Am 01.12.2013 14:13, schrieb Marcel Apfelbaum:
> >> On Fri, 2013-11-29 at 10:43 +0100, address@hidden wrote:
> >>> From: Markus Armbruster <address@hidden>
> >>>
> >>> 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 <address@hidden>
> >> 
> >> 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.
It was merely a design (and understanding) question, if we prefer to enforce
such things and not rely on future work to comply with rules defined in 
comments.
Though I am curios what others think about this specific scenario?

Worst case scenario: the coder forgets about it, the reviewers don't catch
this, the initialization code does not ensure the property is set and
the device is added with an "unhealthy" state. But I suppose such a scenario
would be caught early in the development cycle and is not a real issue. 

Markus, thanks for the explanations,
Marcel

> 
> [...]