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

[Paolo Bonzini]

Il 21/02/2014 13:45, Stefan Hajnoczi ha scritto:
> > This is okay; object_property_add_link reference count takes ownership of
> > the original value of the pointer.
>
> Here's an example:
>
> static void pxa2xx_pcmcia_initfn(Object *obj)
> int pxa2xx_pcmcia_attach(void *opaque, PCMCIACardState *card)
>
> On one hand "card" is a link.  On the other hand we manually assign to
> s->card without using object_property_set_link() and without
> object_ref(card).
>
> This is broken.

Actually, what you showed is fine---ugly, but fine.

It means is that pxa2xx_pcmcia_attach has taken ownership of a reference 
from its caller.  It is ugly because it violates abstraction.  And 
pxa2xx_pcmcia_detach leaks the object because it doesn't object_unref() 
the card.  But it is fine, and no one calls pxa2xx_pcmcia_detach anyway. ;)

> We're abusing the link property because the
> pxa2xx_pcmcia_attach() function is semantically different from
> object_property_set_link().  What's worse is that you can still call
> object_property_set_link() and it will not perform the extra steps
> that pxa2xx_pcmcia_attach() is taking to raise an IRQ and prevent
> attaching to an already attached slot.

*This* is broken, and is exactly why we need link setters.  Adding a 
link setter and using it in pxa2xx_pcmcia_attach/detach would fix all 
the problems here.

Of course, the question is how one would go testing this thing.

> > 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).
>
> Yes, this would allow the weird pxa2xx example to behavior itself
> better because _attach() would become the set() callback function.

Exactly.

> > > 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.
>
> No, virtio-rng-pci has no rng qdev property.  The user cannot set it
> on the command-line:

Sure, it has no *qdev* property, but lo and behold:

    qemu-system-x86_64 \
        -object rng-random,filename=/dev/random,id=rng0 \
        -device virtio-rng-pci,rng=rng0

This is why I believe we want static properties in QOM by the way, not 
just in qdev.  So that this rng property can be documented and not just 
magic.

/me searches for an appropriate Star Wars quote

If once you start down the dark path, forever will it dominate your 
destiny; consume you, it will! As it did Obi-Wan's apprentice.

Paolo

ps: You're fulfilling your destiny, Anakin. Become my apprentice. Learn 
to use the Dark Side of the Force.


> > > 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.
>
> Okay, I'll start working on that and use "x-iothread" in the meantime
> for this series.
>
> Stefan