qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Paolo Bonzini <pbonzini@redhat.com>
To: Anthony Liguori <aliguori@us.ibm.com>
Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org,
	Liu Ping Fan <qemulist@gmail.com>
Subject: Re: [Qemu-devel] [PATCH 1.3 1/5] qom: fix refcount of non-heap-allocated objects
Date: Mon, 26 Nov 2012 17:08:20 +0100	[thread overview]
Message-ID: <50B393F4.5000808@redhat.com> (raw)
In-Reply-To: <87ehjgpdoy.fsf@codemonkey.ws>

Il 26/11/2012 16:49, Anthony Liguori ha scritto:
> But object_property_add_child() will take a reference.
> When the parent object goes away, this will cause that reference to get
> dropped and ultimately the child object to be destroyed.

This is still wrong if you have:

  object_init(&obj->subobj, ...)
  foo(&obj->subobj);
  object_property_add_child()

where foo() does a ref+unref pair (see the pattern of scsi_req_enqueue
for example, even though it's not QOM).

The object's memory area is being kept live by the parent, so it
logically has a nonzero reference count.

We don't have any example of embedding yet in the tree, except for
buses, so I think we are somewhat free to set the rules.

As I said elsewhere in the thread, the rule I'd prefer the most is "it
doesn't matter whether an object is statically- or
dynamically-allocated".  That is, even for an embedded object which you
create in instance_init, you should object_unref it explicitly, either
in instance_finalize or after initializing it (as you prefer).

Note that instance_finalize will anyway have an object_unparent of the
embedded object, in order to remove any cyclical references, so it's not
a big change.

> IOW, this change causes embedded objects to get leaked AFAICT.

Hmm, patch 4 in the series offsets this by doing

 void qbus_free(BusState *bus)
 {
-    if (bus->qom_allocated) {
-        object_delete(OBJECT(bus));
-    } else {
-        object_finalize(OBJECT(bus));
-        if (bus->glib_allocated) {
-            g_free(bus);
-        }
-    }
+    object_delete(OBJECT(bus));
 }

So at the end of the series there is no leak.

Paolo

  reply	other threads:[~2012-11-26 16:08 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-23  8:47 [Qemu-devel] [PATCH 1.3 0/5] QOM/qdev lifetime fixes Paolo Bonzini
2012-11-23  8:47 ` [Qemu-devel] [PATCH 1.3 1/5] qom: fix refcount of non-heap-allocated objects Paolo Bonzini
2012-11-23 16:49   ` Andreas Färber
2012-11-26 15:49   ` Anthony Liguori
2012-11-26 16:08     ` Paolo Bonzini [this message]
2012-11-23  8:47 ` [Qemu-devel] [PATCH 1.3 2/5] qdev: move bus removal to object_unparent Paolo Bonzini
2012-11-23 16:52   ` Andreas Färber
2012-11-27  1:02   ` Andreas Färber
2012-11-23  8:47 ` [Qemu-devel] [PATCH 1.3 3/5] qom: make object_delete usable for statically-allocated objects Paolo Bonzini
2012-11-23 17:02   ` Andreas Färber
2012-11-23 17:10     ` Paolo Bonzini
2012-11-23  8:47 ` [Qemu-devel] [PATCH 1.3 4/5] qdev: simplify (de)allocation of buses Paolo Bonzini
2012-11-23 17:07   ` Andreas Färber
2012-11-23 17:16     ` Paolo Bonzini
2012-11-23  8:47 ` [Qemu-devel] [PATCH 1.3 5/5] qom: make object_finalize static Paolo Bonzini
2012-11-23 17:12   ` Andreas Färber
2012-11-23 17:32     ` Paolo Bonzini
2012-11-26 21:47 ` [Qemu-devel] [PATCH 1.3 0/5] QOM/qdev lifetime fixes Anthony Liguori

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=50B393F4.5000808@redhat.com \
    --to=pbonzini@redhat.com \
    --cc=aliguori@us.ibm.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemulist@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).