From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42745) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yv76g-00021b-FQ for qemu-devel@nongnu.org; Wed, 20 May 2015 12:44:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yv76c-0005hV-72 for qemu-devel@nongnu.org; Wed, 20 May 2015 12:44:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52812) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yv76b-0005hO-VQ for qemu-devel@nongnu.org; Wed, 20 May 2015 12:44:30 -0400 Date: Wed, 20 May 2015 13:44:26 -0300 From: Eduardo Habkost Message-ID: <20150520164426.GN17796@thinpad.lan.raisama.net> References: <1431533649-23115-6-git-send-email-berrange@redhat.com> <555B5C2E.5050903@suse.de> <20150519155528.GF8535@redhat.com> <555B6099.4060803@redhat.com> <20150520144419.GE28075@thinpad.lan.raisama.net> <20150520151803.GA23989@redhat.com> <20150520160621.GL17796@thinpad.lan.raisama.net> <555CB235.7020904@suse.de> <20150520162212.GM17796@thinpad.lan.raisama.net> <555CB54F.7000703@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <555CB54F.7000703@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Paolo Bonzini , qemu-devel@nongnu.org On Wed, May 20, 2015 at 06:24:47PM +0200, Andreas F=E4rber wrote: > Am 20.05.2015 um 18:22 schrieb Eduardo Habkost: > > On Wed, May 20, 2015 at 06:11:33PM +0200, Andreas F=E4rber wrote: > >> Am 20.05.2015 um 18:06 schrieb Eduardo Habkost: > >>> On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote: > >>>> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote: > >>>>> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote: > >>>>>> On 19/05/2015 17:55, Daniel P. Berrange wrote: > >>>>>>> Paolo told me on previous posting that object_property_add_chil= d() > >>>>>>> holds a reference on 'obj' for as long as it is registered in t= he > >>>>>>> object hierarchy composition. So it sufficient to rely on that = long > >>>>>>> term reference, and let the caller dispose of the object by cal= ling > >>>>>>> object_unparent(obj) when finally done. > >>>>>> > >>>>>> For an example of the same pattern: > >>>>>> > >>>>>> DeviceState *qdev_try_create(BusState *bus, const char *type) > >>>>>> { > >>>>>> DeviceState *dev; > >>>>>> > >>>>>> if (object_class_by_name(type) =3D=3D NULL) { > >>>>>> return NULL; > >>>>>> } > >>>>>> dev =3D DEVICE(object_new(type)); > >>>>>> if (!dev) { > >>>>>> return NULL; > >>>>>> } > >>>>>> > >>>>>> if (!bus) { > >>>>>> bus =3D sysbus_get_default(); > >>>>>> } > >>>>>> > >>>>>> qdev_set_parent_bus(dev, bus); > >>>>>> object_unref(OBJECT(dev)); > >>>>>> return dev; > >>>>>> } > >>>>>> > >>>>>> Effectively this is idea as GObject's "floating reference". > >>>>>> qdev_set_parent_bus (in qdev_try_create) and object_property_add= _child > >>>>>> (in Daniel's patches) "sink" the floating reference by doing > >>>>>> object_unref. If we had floating references, the object would b= e > >>>>>> returned to the caller unref'ed anyway. > >>>>> > >>>>> I was agreeing with Andreas at first (because it would make the > >>>>> reference ownership rules simpler and easier to understand), unti= l I > >>>>> noticed that every call of qdev_try_create() and object_resolve_p= ath() > >>>>> in the code would need an additional object_unref() call if we di= dn't > >>>>> use this pattern.changes > >>>>> > >>>>> But it bothers me that this exceptional behavior is not documente= d on > >>>>> neither qdev_try_create() or object_resolve_path(). > >>>>> > >>>>>> > >>>>>> Of course, the reference can go away via QMP. But that will onl= y happen > >>>>>> after the caller would have called object_unref itself. > >>>>> > >>>>> But the caller won't ever call object_unref() because it doesn't = own any > >>>>> reference, right? In this case, can we clarify the rules about ho= w long > >>>>> can callers safely expect the object to stay around? Can the obje= ct be > >>>>> disposed in another thread? Can it be disposed only when some spe= cific > >>>>> events happen? > >>>> > >>>> In the inline docs for object_new_with_props I wrote > >>>> > >>>> * The returned object will have one stable reference maintained > >>>> * for as long as it is present in the object hierarchy. > >>>> > >>>> We could expand it to explicitly say that 'object_unparent' is req= uired > >>>> to remove the object from the hierarchy and free it. > >>> > >>> What's missing to me is some clarification on how long it is safe t= o > >>> assume that the object won't be removed from the hierarchy by other > >>> code. > >> > >> There is no guarantee. Therefore any caller of object_resolve_path() > >> must ref and unref as needed if doing more than a single operation, = such > >> as setting a link target. > >=20 > > So, does that mean zero guarantee, or guarantee for "a single > > operation"? It can't be both. > >=20 > > (And how exactly "a single operation" should be defined?) >=20 > I see your point, after all object_ref() is a single instruction, too. >=20 > We could of course change it to return a ref'ed object, but that would > require a lot of code review and probably unrefs. Mostly for objects > that don't go away, but still. I don't think we should change it to return a ref'ed object, the existing "floating reference" pattern is useful and keeps code simpler. But even if we have plans to change it later, it would be nice to clarify what are the assumptions/guarantees of the existing code. --=20 Eduardo