Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type

[Paolo Bonzini <pbonzini@redhat.com>]

Il 21/02/2014 12:03, Stefan Hajnoczi ha scritto:
> On Thu, Feb 20, 2014 at 01:58:06PM +0100, Paolo Bonzini wrote:
>>> @@ -151,6 +152,8 @@ extern PropertyInfo qdev_prop_arraylen;
>>>     DEFINE_PROP(_n, _s, _f, qdev_prop_vlan, NICPeers)
>>> #define DEFINE_PROP_DRIVE(_n, _s, _f) \
>>>     DEFINE_PROP(_n, _s, _f, qdev_prop_drive, BlockDriverState *)
>>> +#define DEFINE_PROP_IOTHREAD(_n, _s, _f)             \
>>> +    DEFINE_PROP(_n, _s, _f, qdev_prop_iothread, IOThread *)
>>> #define DEFINE_PROP_MACADDR(_n, _s, _f)         \
>>>     DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr)
>>> #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
>>>
>>
>> Should become a link sooner rather than later, but I'm not holding
>> the series for this.
>
> I don't mind doing the work but I don't quite understand it:

I won't claim I understand it 100% either, in fact it is why I don't 
think it should block the series.  But we have actual link users and 
thus actual bugs, which we should fix.

> Links are a special QOM property type: a unidirectional relationship
> where the property holds the path and a reference to another object.
>
> I don't understand how the link's reference is released since
> object_property_add_link() internally doesn't pass a release() function
> pointer to object_property_add().  I also don't see callers explicitly
> calling object_unref() on their link pointer.  Any ideas?

Bug, I guess.

> I'm concerned that existing object_property_add_link() users are
> assigning the link pointer without incrementing the reference count like
> object_set_link_property() would.  That sounds like a recipe for
> disaster if someone uses qom-set or equivalent.

This is okay; object_property_add_link reference count takes ownership 
of the original value of the pointer.

The real disaster is that links cannot be "locked" at realize time.  For 
this to happen, links need to have a setter like object_property_add_str 
(not sure if they need a getter).

> The rng device examples don't seem to help because there is no way to
> specify the rng backend via a qdev property (we always create a default
> backend).  I need to be able to specify the object via a qdev property
> to the virtio-blk-pci device.

You can do that, see virtio-rng-pci.  It creates a link and forwards 
that to virtio-rng.

> Do I need to define a link<> qdev property:
> DEFINE_PROP_LINK("iothread", _state, _field.iothread, TYPE_IOTHREAD, IOThread *)

Perhaps, but to do that we need to first fix object_property_add_link.

Paolo