From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58048) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T4Hy3-0007tL-IV for qemu-devel@nongnu.org; Wed, 22 Aug 2012 16:56:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T4Hy2-0006IQ-Fa for qemu-devel@nongnu.org; Wed, 22 Aug 2012 16:55:59 -0400 Received: from mail-wi0-f181.google.com ([209.85.212.181]:47363) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T4Hy2-0006Gz-8z for qemu-devel@nongnu.org; Wed, 22 Aug 2012 16:55:58 -0400 Received: by wibhm2 with SMTP id hm2so52562wib.10 for ; Wed, 22 Aug 2012 13:55:57 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <5035475A.5070409@redhat.com> Date: Wed, 22 Aug 2012 22:55:54 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1345604562-27289-1-git-send-email-qemulist@gmail.com> <5034CA4C.7020706@redhat.com> <877gsqlw96.fsf@codemonkey.ws> In-Reply-To: <877gsqlw96.fsf@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qom: removal of link property need to release its target List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Liu Ping Fan , Liu Ping Fan , qemu-devel@nongnu.org Il 22/08/2012 18:36, Anthony Liguori ha scritto: > We can fix this by adding an extra reference in add_link but this > creates yet another problem with hotplug. Specificially, qdev_free() > asserts that ref > 0 because there is now a reference being held by the > bus. I think that's correct. Unplugging needs to remove the circular reference before object_delete is called. > This is the same problem we have with object_unparent. No, it's not. Parent-to-child is a one-way relationship, it cannot have circular references. It is also a "cosmetic" relationship, in that the void * is usually not accessed except when using QOM paths. If child properties are known to the parent (they are not, for example, when the parent is a TYPE_CONTAINER), the parent knows how to reach the child using a C expression. So unparenting at object_delete time (not before) makes sense. > The key problem here is how unplug is implemented. Unplug wants to be > both synchronous and asynchronous. > > I think we need to do the following: > > 1) Move object_unparent to qdev_device_del (the parent is added by > qdev_device_add so this is quite logical). There is no qdev_device_del, but there is qdev_unplug. We could rename qdev_unplug to qdev_request_unplug, and qdev_simple_unplug_cb to qdev_unplug. > 2) Make DeviceState::unplug *never* call qdev_free(). Right. It should always go through qdev_simple_unplug_cb. > 3) Add an "unplugged" NotifierList to DeviceState. Is this really needed? > 4) Change the various hotplug consumers to call qdev_set_parent_bus() to > NULL to unplug the device from the bus. Change qdev_set_parent_bus() > to allow this and remove the bus link and invoke the unplugged notifier. This too, is it needed?... qdev_simple_unplug_cb could simply be the place where you call qdev_set_parent_bus(qdev, NULL), before qdev_free. That would break the circular link and keep object_delete() happy. > 5) Change qdev_device_del() to add a notifier to the object that calls > object_unparent() and object_unref. No need. > 6) Rename DeviceState::unplug to DeviceState::request_unplug Cosmetic, but I agree. :) > 7) Take Ping Fan's patch + another patch to add a reference count in > object_property_add_link Yes. BTW, the patch to fix usb_del is on its way. Paolo