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(-)
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)
--
2.11.0.295.gd7dffce1c.dirty
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.
--
Marc-André Lureau
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. -- Marc-André Lureau
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
© 2016 - 2025 Red Hat, Inc.