On Fri, Feb 21, 2014 at 12:33 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: > 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.
Here's an example: static void pxa2xx_pcmcia_initfn(Object *obj) { ... object_property_add_link(obj, "card", TYPE_PCMCIA_CARD, (Object **)&s->card, NULL); } /* Insert a new card into a slot */ int pxa2xx_pcmcia_attach(void *opaque, PCMCIACardState *card) { PXA2xxPCMCIAState *s = (PXA2xxPCMCIAState *) opaque; PCMCIACardClass *pcc; if (s->slot.attached) { return -EEXIST; } if (s->cd_irq) { qemu_irq_raise(s->cd_irq); } s->card = card; pcc = PCMCIA_CARD_GET_CLASS(s->card); s->slot.attached = true; s->card->slot = &s->slot; pcc->attach(s->card); return 0; } 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. 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. Either I don't understand QOM links or pxa2xx is broken code. > 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. >> 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: #define DEFINE_VIRTIO_RNG_PROPERTIES(_state, _conf_field) \ DEFINE_PROP_UINT64("max-bytes", _state, _conf_field.max_bytes, \ INT64_MAX), \ DEFINE_PROP_UINT32("period", _state, _conf_field.period_ms, 1 << 16) And here's the proof: $ x86_64-softmmu/qemu-system-x86_64 -device virtio-rng-pci,\? virtio-rng-pci.indirect_desc=on/off virtio-rng-pci.event_idx=on/off virtio-rng-pci.max-bytes=uint64 virtio-rng-pci.period=uint32 virtio-rng-pci.addr=pci-devfn virtio-rng-pci.romfile=str virtio-rng-pci.rombar=uint32 virtio-rng-pci.multifunction=on/off virtio-rng-pci.command_serr_enable=on/off >> 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