From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45102) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGpjG-0002z4-UH for qemu-devel@nongnu.org; Fri, 21 Feb 2014 08:01:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WGpj8-0005l0-4x for qemu-devel@nongnu.org; Fri, 21 Feb 2014 08:01:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40839) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WGpj7-0005kh-UJ for qemu-devel@nongnu.org; Fri, 21 Feb 2014 08:01:14 -0500 Message-ID: <53074E0F.1040006@redhat.com> Date: Fri, 21 Feb 2014 14:01:03 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1392900630-17608-1-git-send-email-stefanha@redhat.com> <1392900630-17608-6-git-send-email-stefanha@redhat.com> <5305FBDE.8030004@redhat.com> <20140221110317.GC17890@stefanha-thinkpad.redhat.com> <53073992.70803@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Fam Zheng , qemu-devel , Stefan Hajnoczi , Michael Roth 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 >