| Message ID | 20170202145141.17138-7-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/02/2017 15:51, Marc-André Lureau wrote: > + if (QTAILQ_IN_USE(chr, next)) { > + QTAILQ_REMOVE(&chardevs, chr, next); > + } > + if (OBJECT(chr)->parent) { > + object_unparent(OBJECT(chr)); > + } else { > + object_unref(OBJECT(chr)); > + } What's the case where the "else" is used? Probably qemu_chr_delete callers should be changed to use object_unparent or object_unref directly. Paolo
Hi ----- Original Message ----- > > > On 02/02/2017 15:51, Marc-André Lureau wrote: > > + if (QTAILQ_IN_USE(chr, next)) { > > + QTAILQ_REMOVE(&chardevs, chr, next); > > + } > > + if (OBJECT(chr)->parent) { > > + object_unparent(OBJECT(chr)); > > + } else { > > + object_unref(OBJECT(chr)); > > + } > > What's the case where the "else" is used? Probably qemu_chr_delete > callers should be changed to use object_unparent or object_unref directly. I thought about that, but calling object_unparent() seems weird, since callers aren't much aware of the fact that chardev are added or not to a container (useless distinction imho). I wish the last object_unref() would automatically unparent, if the object has a parent. Would that be acceptable?
On 07/02/2017 21:03, Marc-André Lureau wrote: > Hi > > ----- Original Message ----- >> >> >> On 02/02/2017 15:51, Marc-André Lureau wrote: >>> + if (QTAILQ_IN_USE(chr, next)) { >>> + QTAILQ_REMOVE(&chardevs, chr, next); >>> + } >>> + if (OBJECT(chr)->parent) { >>> + object_unparent(OBJECT(chr)); >>> + } else { >>> + object_unref(OBJECT(chr)); >>> + } >> >> What's the case where the "else" is used? Probably qemu_chr_delete >> callers should be changed to use object_unparent or object_unref directly. > > I thought about that, but calling object_unparent() seems weird, > since callers aren't much aware of the fact that chardev are added or not to a > container (useless distinction imho). I wish the last object_unref() > would automatically unparent, if the object has a parent. Would that be > acceptable? There is a distinction between the two. The idea is that unparent removes all persistent references in the object tree, while unref only removes transient references. So for example unparent will detach a device from its bus. Unparent is basically exploiting the object tree in order to simplify the handling of reference cycles. Once you add an object with object_property_add_child, you probably should remove any transient references you have (such as the one you got with object_new) and from that point on use object_unparent only. Paolo
Hi On Fri, Feb 10, 2017 at 4:18 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 07/02/2017 21:03, Marc-André Lureau wrote: > > Hi > > > > ----- Original Message ----- > >> > >> > >> On 02/02/2017 15:51, Marc-André Lureau wrote: > >>> + if (QTAILQ_IN_USE(chr, next)) { > >>> + QTAILQ_REMOVE(&chardevs, chr, next); > >>> + } > >>> + if (OBJECT(chr)->parent) { > >>> + object_unparent(OBJECT(chr)); > >>> + } else { > >>> + object_unref(OBJECT(chr)); > >>> + } > >> > >> What's the case where the "else" is used? Probably qemu_chr_delete > >> callers should be changed to use object_unparent or object_unref > directly. > > > > I thought about that, but calling object_unparent() seems weird, > > since callers aren't much aware of the fact that chardev are added or > not to a > > container (useless distinction imho). I wish the last object_unref() > > would automatically unparent, if the object has a parent. Would that be > > acceptable? > > There is a distinction between the two. The idea is that unparent > removes all persistent references in the object tree, while unref only > removes transient references. So for example unparent will detach a > device from its bus. Unparent is basically exploiting the object tree > in order to simplify the handling of reference cycles. > > Once you add an object with object_property_add_child, you probably > should remove any transient references you have (such as the one you got > with object_new) and from that point on use object_unparent only. > But if you unparent with the last ref, you remove the burden of knowing if the object has been parented from the user. I don't see why that would conflict with object_unparent(), you could still unparent(), and keep the object referenced somewhere else. The two are not incompatible to me. Afaik, most widget/hierarchy API work like that, the last unref will implicitely unparent.
On 10/02/2017 13:14, Marc-André Lureau wrote: > Hi > > On Fri, Feb 10, 2017 at 4:18 AM Paolo Bonzini <pbonzini@redhat.com > <mailto:pbonzini@redhat.com>> wrote: > > > > On 07/02/2017 21:03, Marc-André Lureau wrote: > > Hi > > > > ----- Original Message ----- > >> > >> > >> On 02/02/2017 15:51, Marc-André Lureau wrote: > >>> + if (QTAILQ_IN_USE(chr, next)) { > >>> + QTAILQ_REMOVE(&chardevs, chr, next); > >>> + } > >>> + if (OBJECT(chr)->parent) { > >>> + object_unparent(OBJECT(chr)); > >>> + } else { > >>> + object_unref(OBJECT(chr)); > >>> + } > >> > >> What's the case where the "else" is used? Probably qemu_chr_delete > >> callers should be changed to use object_unparent or object_unref > directly. > > > > I thought about that, but calling object_unparent() seems weird, > > since callers aren't much aware of the fact that chardev are added > or not to a > > container (useless distinction imho). I wish the last object_unref() > > would automatically unparent, if the object has a parent. Would > that be > > acceptable? > > There is a distinction between the two. The idea is that unparent > removes all persistent references in the object tree, while unref only > removes transient references. So for example unparent will detach a > device from its bus. Unparent is basically exploiting the object tree > in order to simplify the handling of reference cycles. > > Once you add an object with object_property_add_child, you probably > should remove any transient references you have (such as the one you got > with object_new) and from that point on use object_unparent only. > > > But if you unparent with the last ref, you remove the burden of knowing > if the object has been parented from the user. I don't see why that > would conflict with object_unparent(), you could still unparent(), and > keep the object referenced somewhere else. Isn't that exactly why you want them to be different? unparent can do much more than unref, for example in the case of a device it will also unrealize it and destroy all buses underneath it. Because the device and bus have a circular reference, you cannot trigger the magic unparent behavior just by unref'ing the device. There are just two cases: - destruction immediately after creation, e.g. on error: new/unref - successful creation: new/add_child/unref, unparent when deleting and it's simpler to remember these two than to add magic behavior. > The two are not incompatible > to me. Afaik, most widget/hierarchy API work like that, the last unref > will implicitely unparent. LibreOffice's has some similarity with QOM, search for "dispose" at https://people.gnome.org/~michael/blog/2015-08-05-under-the-hood-5-0.html Paolo
Hi On Fri, Feb 10, 2017 at 4:26 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > But if you unparent with the last ref, you remove the burden of knowing > > if the object has been parented from the user. I don't see why that > > would conflict with object_unparent(), you could still unparent(), and > > keep the object referenced somewhere else. > > Isn't that exactly why you want them to be different? unparent can do > much more than unref, for example in the case of a device it will also > unrealize it and destroy all buses underneath it. Because the device > Shouldn't the last unref also unrealize/destroy everything? > and bus have a circular reference, you cannot trigger the magic unparent > behavior just by unref'ing the device. > Whoo, we have circular references on purposes? Is this lifecycle documented somewhere? I wonder the rationale behind it. > > There are just two cases: > > - destruction immediately after creation, e.g. on error: new/unref > > - successful creation: new/add_child/unref, unparent when deleting > > and it's simpler to remember these two than to add magic behavior. > > That doesn't change the problem, the user may not know if the object is parented. So you have to pass this information along. > > The two are not incompatible > > to me. Afaik, most widget/hierarchy API work like that, the last unref > > will implicitely unparent. > > LibreOffice's has some similarity with QOM, search for "dispose" at > https://people.gnome.org/~michael/blog/2015-08-05-under-the-hood-5-0.html > > I don't see much parallel with the discussion here except that they had a complicated model and tried to simplify it. That's what I also try to do.
On 10/02/2017 13:59, Marc-André Lureau wrote: > > But if you unparent with the last ref, you remove the burden of knowing > > if the object has been parented from the user. I don't see why that > > would conflict with object_unparent(), you could still unparent(), and > > keep the object referenced somewhere else. > > Isn't that exactly why you want them to be different? unparent can do > much more than unref, for example in the case of a device it will also > unrealize it and destroy all buses underneath it. Because the device > > Shouldn't the last unref also unrealize/destroy everything? How could someone say they have the "last unref"? They didn't get that reference from anywhere, the reference is owned by the parent object's child property. > and bus have a circular reference, you cannot trigger the magic unparent > behavior just by unref'ing the device. > > Whoo, we have circular references on purposes? Is this lifecycle > documented somewhere? I wonder the rationale behind it. The same as Libreoffice: we had a model with no reference counting (just "qdev_free") and we tried to adapt it to QOM's reference counting model, in our case by exploiting the object tree. > There are just two cases: > > - destruction immediately after creation, e.g. on error: new/unref > > - successful creation: new/add_child/unref, unparent when deleting > > and it's simpler to remember these two than to add magic behavior. > > That doesn't change the problem, the user may not know if the object is > parented. If the user got a reference for himself via object_ref or object_new, it must use object_unref. If the user wants to remove public knowledge of an object, then they must use object_unparent. If the object is not parented, there's a violation of QOM rules somewhere. Paolo
diff --git a/include/sysemu/char.h b/include/sysemu/char.h index a30ff3fa80..e3f3a10d17 100644 --- a/include/sysemu/char.h +++ b/include/sysemu/char.h @@ -490,7 +490,8 @@ typedef struct ChardevClass { } ChardevClass; Chardev *qemu_chardev_new(const char *id, const char *typename, - ChardevBackend *backend, Error **errp); + ChardevBackend *backend, bool enlist, + Error **errp); extern int term_escape_char; diff --git a/chardev/char.c b/chardev/char.c index 9f02c6d5f1..5a12a79c3b 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -45,6 +45,11 @@ static QTAILQ_HEAD(ChardevHead, Chardev) chardevs = QTAILQ_HEAD_INITIALIZER(chardevs); +static Object *get_chardevs_root(void) +{ + return container_get(object_get_root(), "/chardevs"); +} + void qemu_chr_be_event(Chardev *s, int event) { CharBackend *be = s->be; @@ -804,7 +809,7 @@ static Chardev *qemu_chardev_add(const char *id, const char *typename, return NULL; } - chr = qemu_chardev_new(id, typename, backend, errp); + chr = qemu_chardev_new(id, typename, backend, true, errp); if (!chr) { return NULL; } @@ -1061,8 +1066,14 @@ void qemu_chr_fe_disconnect(CharBackend *be) void qemu_chr_delete(Chardev *chr) { - QTAILQ_REMOVE(&chardevs, chr, next); - object_unref(OBJECT(chr)); + if (QTAILQ_IN_USE(chr, next)) { + QTAILQ_REMOVE(&chardevs, chr, next); + } + if (OBJECT(chr)->parent) { + object_unparent(OBJECT(chr)); + } else { + object_unref(OBJECT(chr)); + } } ChardevInfoList *qmp_query_chardev(Error **errp) @@ -1224,22 +1235,33 @@ void qemu_chr_set_feature(Chardev *chr, } Chardev *qemu_chardev_new(const char *id, const char *typename, - ChardevBackend *backend, Error **errp) + ChardevBackend *backend, bool enlist, + Error **errp) { + Object *obj; Chardev *chr = NULL; Error *local_err = NULL; bool be_opened = true; assert(g_str_has_prefix(typename, "chardev-")); - chr = CHARDEV(object_new(typename)); + if (enlist) { + obj = object_new_with_props(typename, get_chardevs_root(), + id, &local_err, NULL); + } else { + obj = object_new(typename); + } + if (local_err) { + assert(!obj); + goto end; + } + + chr = CHARDEV(obj); chr->label = g_strdup(id); qemu_char_open(chr, backend, &be_opened, &local_err); if (local_err) { - error_propagate(errp, local_err); - object_unref(OBJECT(chr)); - return NULL; + goto end; } if (!chr->filename) { @@ -1249,6 +1271,14 @@ Chardev *qemu_chardev_new(const char *id, const char *typename, qemu_chr_be_event(chr, CHR_EVENT_OPENED); } +end: + if (local_err) { + error_propagate(errp, local_err); + if (chr) { + qemu_chr_delete(chr); + } + return NULL; + } return chr; } diff --git a/gdbstub.c b/gdbstub.c index 755a8e378d..613ac48fed 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -1793,7 +1793,7 @@ int gdbserver_start(const char *device) /* Initialize a monitor terminal for gdb */ mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB, - NULL, &error_abort); + NULL, false, &error_abort); monitor_init(mon_chr, 0); } else { if (qemu_chr_fe_get_driver(&s->chr)) { diff --git a/hw/bt/hci-csr.c b/hw/bt/hci-csr.c index 3c193848fc..9c211e89c4 100644 --- a/hw/bt/hci-csr.c +++ b/hw/bt/hci-csr.c @@ -503,7 +503,7 @@ static const TypeInfo char_hci_type_info = { Chardev *uart_hci_init(void) { return qemu_chardev_new(NULL, TYPE_CHARDEV_HCI, - NULL, &error_abort); + NULL, false, &error_abort); } static void register_types(void)
Add a /chardevs container object to hold the list of chardevs. (Note: QTAILQ chardevs is going away in the following commits) Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- include/sysemu/char.h | 3 ++- chardev/char.c | 46 ++++++++++++++++++++++++++++++++++++++-------- gdbstub.c | 2 +- hw/bt/hci-csr.c | 2 +- 4 files changed, 42 insertions(+), 11 deletions(-)