[Qemu-devel] [PATCH v2 0/4] iothread: allow to create internal iothreads

[Qemu-devel] [PATCH v2 0/4] iothread: allow to create internal iothreads

[Peter Xu]

v2:
- add one patch to provide object_get_internal_root() [Daniel]
- patch 2: use the new object_get_internal_root()
- patch 3: fix commit message, "reentrant" is wrongly used by me. it
  should be "called multiple times"; move iothread->ctx check into
  iothread_stop() [Fam]
- patch 4: add one paragraph in commit message, mention about the glib
  issue. [Fam]

When trying to support monitor OOB (out-of-band) commands, I found
that the monitor IO thread I did looks just like iothread.  It would
be best if I can use iothread directly.  However it seems that it was
mostly used by "-object iothread" before but not friendly to internal
usages.  This series tries to export essential functions to do it.

Also, I think patch 2 also fixes a bug in iothread_stop().

Please review. Thanks.

Peter Xu (4):
  qom: provide root container for internal objs
  iothread: provide helpers for internal use
  iothread: export iothread_stop()
  iothread: delay the context release to finalize

 include/qom/object.h      | 10 ++++++++++
 include/sysemu/iothread.h |  9 +++++++++
 iothread.c                | 46 ++++++++++++++++++++++++++++++++++++----------
 qom/object.c              |  5 +++++
 4 files changed, 60 insertions(+), 10 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Peter Xu]

We have object_get_objects_root() to keep user created objects, however
no place for objects that will be used internally.  Create such a
container for internal objects.

CC: Andreas Färber <afaerber@suse.de>
CC: Markus Armbruster <armbru@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qom/object.h | 10 ++++++++++
 qom/object.c         |  5 +++++
 2 files changed, 15 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index f3e5cff..f567052 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1214,6 +1214,16 @@ Object *object_get_root(void);
 Object *object_get_objects_root(void);
 
 /**
+ * object_get_internal_root:
+ *
+ * Get the container object that holds internally used object
+ * instances. This is the object at path "/internal-objects"
+ *
+ * Returns: the internal object container
+ */
+Object *object_get_internal_root(void);
+
+/**
  * object_get_canonical_path_component:
  *
  * Returns: The final component in the object's canonical path.  The canonical
diff --git a/qom/object.c b/qom/object.c
index 3e18537..857cee7 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
     return container_get(object_get_root(), "/objects");
 }
 
+Object *object_get_internal_root(void)
+{
+    return container_get(object_get_root(), "/internal-objects");
+}
+
 static void object_get_child_property(Object *obj, Visitor *v,
                                       const char *name, void *opaque,
                                       Error **errp)
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/4] iothread: provide helpers for internal use

[Peter Xu]

IOThread is a general framework that contains IO loop environment and a
real thread behind.  It's also good to be used internally inside qemu.
Provide some helpers for it to create iothreads to be used internally.

Put all the internal used iothreads into the internal object container.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/sysemu/iothread.h |  8 ++++++++
 iothread.c                | 16 ++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index d2985b3..b07663f 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -46,4 +46,12 @@ AioContext *iothread_get_aio_context(IOThread *iothread);
 void iothread_stop_all(void);
 GMainContext *iothread_get_g_main_context(IOThread *iothread);
 
+/*
+ * Helpers used to allocate iothreads for internal use.  These
+ * iothreads will not be seen by monitor clients when query using
+ * "query-iothreads".
+ */
+IOThread *iothread_create(const char *id, Error **errp);
+void iothread_destroy(IOThread *iothread);
+
 #endif /* IOTHREAD_H */
diff --git a/iothread.c b/iothread.c
index 44c8944..33f996e 100644
--- a/iothread.c
+++ b/iothread.c
@@ -354,3 +354,19 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread)
 
     return iothread->worker_context;
 }
+
+IOThread *iothread_create(const char *id, Error **errp)
+{
+    Object *obj;
+
+    obj = object_new_with_props(TYPE_IOTHREAD,
+                                object_get_internal_root(),
+                                id, errp, NULL);
+
+    return IOTHREAD(obj);
+}
+
+void iothread_destroy(IOThread *iothread)
+{
+    object_unparent(OBJECT(iothread));
+}
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/4] iothread: export iothread_stop()

[Peter Xu]

So that internal iothread users can explicitly stop one iothread without
destroying it.

Since at it, fix iothread_stop() to allow it to be called multiple
times.  Before this patch we may call iothread_stop() more than once on
single iothread, while that may not be correct since qemu_thread_join()
is not allowed to run twice.  From manual of pthread_join():

  Joining with a thread that has previously been joined results in
  undefined behavior.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/sysemu/iothread.h |  1 +
 iothread.c                | 24 ++++++++++++++++--------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index b07663f..110329b 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -52,6 +52,7 @@ GMainContext *iothread_get_g_main_context(IOThread *iothread);
  * "query-iothreads".
  */
 IOThread *iothread_create(const char *id, Error **errp);
+void iothread_stop(IOThread *iothread);
 void iothread_destroy(IOThread *iothread);
 
 #endif /* IOTHREAD_H */
diff --git a/iothread.c b/iothread.c
index 33f996e..b3c092b 100644
--- a/iothread.c
+++ b/iothread.c
@@ -80,13 +80,10 @@ static void *iothread_run(void *opaque)
     return NULL;
 }
 
-static int iothread_stop(Object *object, void *opaque)
+void iothread_stop(IOThread *iothread)
 {
-    IOThread *iothread;
-
-    iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
-    if (!iothread || !iothread->ctx) {
-        return 0;
+    if (!iothread->ctx || iothread->stopping) {
+        return;
     }
     iothread->stopping = true;
     aio_notify(iothread->ctx);
@@ -94,6 +91,17 @@ static int iothread_stop(Object *object, void *opaque)
         g_main_loop_quit(iothread->main_loop);
     }
     qemu_thread_join(&iothread->thread);
+}
+
+static int iothread_stop_iter(Object *object, void *opaque)
+{
+    IOThread *iothread;
+
+    iothread = (IOThread *)object_dynamic_cast(object, TYPE_IOTHREAD);
+    if (!iothread) {
+        return 0;
+    }
+    iothread_stop(iothread);
     return 0;
 }
 
@@ -108,7 +116,7 @@ static void iothread_instance_finalize(Object *obj)
 {
     IOThread *iothread = IOTHREAD(obj);
 
-    iothread_stop(obj, NULL);
+    iothread_stop(iothread);
     qemu_cond_destroy(&iothread->init_done_cond);
     qemu_mutex_destroy(&iothread->init_done_lock);
     if (!iothread->ctx) {
@@ -328,7 +336,7 @@ void iothread_stop_all(void)
         aio_context_release(ctx);
     }
 
-    object_child_foreach(container, iothread_stop, NULL);
+    object_child_foreach(container, iothread_stop_iter, NULL);
 }
 
 static gpointer iothread_g_main_context_init(gpointer opaque)
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/4] iothread: delay the context release to finalize

[Peter Xu]

When gcontext is used with iothread, the context will be destroyed
during iothread_stop().  That's not good since sometimes we would like
to keep the resources until iothread is destroyed, but we may want to
stop the thread before that point.

Delay the destruction of gcontext to iothread finalize.  Then we can do:

  iothread_stop(thread);
  some_cleanup_on_resources();
  iothread_destroy(thread);

We may need this patch if we want to run chardev IOs in iothreads and
hopefully clean them up correctly.  For more specific information,
please see 2b316774f6 ("qemu-char: do not operate on sources from
finalize callbacks").

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 iothread.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/iothread.c b/iothread.c
index b3c092b..27a4288 100644
--- a/iothread.c
+++ b/iothread.c
@@ -71,8 +71,6 @@ static void *iothread_run(void *opaque)
             g_main_loop_unref(loop);
 
             g_main_context_pop_thread_default(iothread->worker_context);
-            g_main_context_unref(iothread->worker_context);
-            iothread->worker_context = NULL;
         }
     }
 
@@ -117,6 +115,10 @@ static void iothread_instance_finalize(Object *obj)
     IOThread *iothread = IOTHREAD(obj);
 
     iothread_stop(iothread);
+    if (iothread->worker_context) {
+        g_main_context_unref(iothread->worker_context);
+        iothread->worker_context = NULL;
+    }
     qemu_cond_destroy(&iothread->init_done_cond);
     qemu_mutex_destroy(&iothread->init_done_lock);
     if (!iothread->ctx) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Andreas Färber]

Am 25.09.2017 um 08:37 schrieb Peter Xu:
> We have object_get_objects_root() to keep user created objects, however
> no place for objects that will be used internally.  Create such a
> container for internal objects.
> 
> CC: Andreas Färber <afaerber@suse.de>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qom/object.h | 10 ++++++++++
>  qom/object.c         |  5 +++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f3e5cff..f567052 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1214,6 +1214,16 @@ Object *object_get_root(void);
>  Object *object_get_objects_root(void);
>  
>  /**
> + * object_get_internal_root:
> + *
> + * Get the container object that holds internally used object
> + * instances. This is the object at path "/internal-objects"
> + *
> + * Returns: the internal object container
> + */
> +Object *object_get_internal_root(void);
> +
> +/**
>   * object_get_canonical_path_component:
>   *
>   * Returns: The final component in the object's canonical path.  The canonical
> diff --git a/qom/object.c b/qom/object.c
> index 3e18537..857cee7 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
>      return container_get(object_get_root(), "/objects");
>  }
>  
> +Object *object_get_internal_root(void)
> +{
> +    return container_get(object_get_root(), "/internal-objects");

Whatever you expose in the QOM tree is no longer internal. Other name?

Regards,
Andreas

> +}
> +
>  static void object_get_child_property(Object *obj, Visitor *v,
>                                        const char *name, void *opaque,
>                                        Error **errp)
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2 3/4] iothread: export iothread_stop()

[Fam Zheng]

On Mon, 09/25 14:37, Peter Xu wrote:
> So that internal iothread users can explicitly stop one iothread without
> destroying it.
> 
> Since at it, fix iothread_stop() to allow it to be called multiple
> times.  Before this patch we may call iothread_stop() more than once on
> single iothread, while that may not be correct since qemu_thread_join()
> is not allowed to run twice.  From manual of pthread_join():
> 
>   Joining with a thread that has previously been joined results in
>   undefined behavior.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Peter Xu]

On Mon, Sep 25, 2017 at 09:14:21AM +0200, Andreas Färber wrote:
> Am 25.09.2017 um 08:37 schrieb Peter Xu:
> > We have object_get_objects_root() to keep user created objects, however
> > no place for objects that will be used internally.  Create such a
> > container for internal objects.
> > 
> > CC: Andreas Färber <afaerber@suse.de>
> > CC: Markus Armbruster <armbru@redhat.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/qom/object.h | 10 ++++++++++
> >  qom/object.c         |  5 +++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index f3e5cff..f567052 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -1214,6 +1214,16 @@ Object *object_get_root(void);
> >  Object *object_get_objects_root(void);
> >  
> >  /**
> > + * object_get_internal_root:
> > + *
> > + * Get the container object that holds internally used object
> > + * instances. This is the object at path "/internal-objects"
> > + *
> > + * Returns: the internal object container
> > + */
> > +Object *object_get_internal_root(void);
> > +
> > +/**
> >   * object_get_canonical_path_component:
> >   *
> >   * Returns: The final component in the object's canonical path.  The canonical
> > diff --git a/qom/object.c b/qom/object.c
> > index 3e18537..857cee7 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
> >      return container_get(object_get_root(), "/objects");
> >  }
> >  
> > +Object *object_get_internal_root(void)
> > +{
> > +    return container_get(object_get_root(), "/internal-objects");
> 
> Whatever you expose in the QOM tree is no longer internal. Other name?

Hi, Andreas,

If you mean "info qom-tree" here, can we still treat it as internal?
Since after all it's not exposed in QMP (while IMHO QMP is the
official protocol for QEMU clients).  And IIUC some HMP commands do
dump some internal structs for debugging purpose (like: "info
ramblock").

Or, do you have any suggestion?

(I did think about something like "hidden-objects", but I believe they
 are not hidden as well if we think they are not internal...)

Thanks,

> 
> Regards,
> Andreas
> 
> > +}
> > +
> >  static void object_get_child_property(Object *obj, Visitor *v,
> >                                        const char *name, void *opaque,
> >                                        Error **errp)
> > 
> 
> 
> -- 
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Manos Pitsidianakis]

[-- Attachment #1: Type: text/plain, Size: 2658 bytes --]

On Mon, Sep 25, 2017 at 04:14:26PM +0800, Peter Xu wrote:
>On Mon, Sep 25, 2017 at 09:14:21AM +0200, Andreas Färber wrote:
>> Am 25.09.2017 um 08:37 schrieb Peter Xu:
>> > We have object_get_objects_root() to keep user created objects, however
>> > no place for objects that will be used internally.  Create such a
>> > container for internal objects.
>> >
>> > CC: Andreas Färber <afaerber@suse.de>
>> > CC: Markus Armbruster <armbru@redhat.com>
>> > CC: Paolo Bonzini <pbonzini@redhat.com>
>> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  include/qom/object.h | 10 ++++++++++
>> >  qom/object.c         |  5 +++++
>> >  2 files changed, 15 insertions(+)
>> >
>> > diff --git a/include/qom/object.h b/include/qom/object.h
>> > index f3e5cff..f567052 100644
>> > --- a/include/qom/object.h
>> > +++ b/include/qom/object.h
>> > @@ -1214,6 +1214,16 @@ Object *object_get_root(void);
>> >  Object *object_get_objects_root(void);
>> >
>> >  /**
>> > + * object_get_internal_root:
>> > + *
>> > + * Get the container object that holds internally used object
>> > + * instances. This is the object at path "/internal-objects"
>> > + *
>> > + * Returns: the internal object container
>> > + */
>> > +Object *object_get_internal_root(void);
>> > +
>> > +/**
>> >   * object_get_canonical_path_component:
>> >   *
>> >   * Returns: The final component in the object's canonical path.  The canonical
>> > diff --git a/qom/object.c b/qom/object.c
>> > index 3e18537..857cee7 100644
>> > --- a/qom/object.c
>> > +++ b/qom/object.c
>> > @@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
>> >      return container_get(object_get_root(), "/objects");
>> >  }
>> >
>> > +Object *object_get_internal_root(void)
>> > +{
>> > +    return container_get(object_get_root(), "/internal-objects");
>>
>> Whatever you expose in the QOM tree is no longer internal. Other name?
>
>Hi, Andreas,
>
>If you mean "info qom-tree" here, can we still treat it as internal?
>Since after all it's not exposed in QMP (while IMHO QMP is the
>official protocol for QEMU clients).  And IIUC some HMP commands do
>dump some internal structs for debugging purpose (like: "info
>ramblock").
>
>Or, do you have any suggestion?
>
>(I did think about something like "hidden-objects", but I believe they
> are not hidden as well if we think they are not internal...)
>
>Thanks,
>
>>
>> Regards,
>> Andreas

If there's no need for internal IOThreads to be together with user 
created ones, a simple queue will be enough (include/qemu/queue.h).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Andreas Färber]

Hi,

Am 25.09.2017 um 10:14 schrieb Peter Xu:
> On Mon, Sep 25, 2017 at 09:14:21AM +0200, Andreas Färber wrote:
>> Am 25.09.2017 um 08:37 schrieb Peter Xu:
>>> We have object_get_objects_root() to keep user created objects, however
>>> no place for objects that will be used internally.  Create such a
>>> container for internal objects.
>>>
>>> CC: Andreas Färber <afaerber@suse.de>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> CC: Paolo Bonzini <pbonzini@redhat.com>
>>> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>  include/qom/object.h | 10 ++++++++++
>>>  qom/object.c         |  5 +++++
>>>  2 files changed, 15 insertions(+)
>>>
>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>> index f3e5cff..f567052 100644
>>> --- a/include/qom/object.h
>>> +++ b/include/qom/object.h
>>> @@ -1214,6 +1214,16 @@ Object *object_get_root(void);
>>>  Object *object_get_objects_root(void);
>>>  
>>>  /**
>>> + * object_get_internal_root:
>>> + *
>>> + * Get the container object that holds internally used object
>>> + * instances. This is the object at path "/internal-objects"
>>> + *
>>> + * Returns: the internal object container
>>> + */
>>> +Object *object_get_internal_root(void);
>>> +
>>> +/**
>>>   * object_get_canonical_path_component:
>>>   *
>>>   * Returns: The final component in the object's canonical path.  The canonical
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 3e18537..857cee7 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
>>>      return container_get(object_get_root(), "/objects");
>>>  }
>>>  
>>> +Object *object_get_internal_root(void)
>>> +{
>>> +    return container_get(object_get_root(), "/internal-objects");
>>
>> Whatever you expose in the QOM tree is no longer internal. Other name?
> 
> Hi, Andreas,
> 
> If you mean "info qom-tree" here, can we still treat it as internal?
> Since after all it's not exposed in QMP (while IMHO QMP is the
> official protocol for QEMU clients).  And IIUC some HMP commands do
> dump some internal structs for debugging purpose (like: "info
> ramblock").
> 
> Or, do you have any suggestion?
> 
> (I did think about something like "hidden-objects", but I believe they
>  are not hidden as well if we think they are not internal...)

I'm on travels and only seeing this 1/4 without context - according to
Manos apparently something about IOThreads.

The reason that certain container groups such as "/machine/peripheral"
exist was more of a legacy reason for migration from qdev, so yes I am
critical of "/hidden-objects" as well. If the objects are not related to
an existing device, you could just place them somewhere outside
"/machine", e.g. directly in the root node or in a container specific to
that use case (/io? /threads?), rather than a new generic bucket of
whatever name. Any objects not under /machine should be close to what
you consider internal.

Note that "info qom-tree" is not the only way to query the objects,
there's some Python QMP scripts as well.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Daniel P. Berrange]

On Mon, Sep 25, 2017 at 09:14:21AM +0200, Andreas Färber wrote:
> Am 25.09.2017 um 08:37 schrieb Peter Xu:
> > We have object_get_objects_root() to keep user created objects, however
> > no place for objects that will be used internally.  Create such a
> > container for internal objects.
> > 
> > CC: Andreas Färber <afaerber@suse.de>
> > CC: Markus Armbruster <armbru@redhat.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/qom/object.h | 10 ++++++++++
> >  qom/object.c         |  5 +++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index f3e5cff..f567052 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -1214,6 +1214,16 @@ Object *object_get_root(void);
> >  Object *object_get_objects_root(void);
> >  
> >  /**
> > + * object_get_internal_root:
> > + *
> > + * Get the container object that holds internally used object
> > + * instances. This is the object at path "/internal-objects"
> > + *
> > + * Returns: the internal object container
> > + */
> > +Object *object_get_internal_root(void);
> > +
> > +/**
> >   * object_get_canonical_path_component:
> >   *
> >   * Returns: The final component in the object's canonical path.  The canonical
> > diff --git a/qom/object.c b/qom/object.c
> > index 3e18537..857cee7 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
> >      return container_get(object_get_root(), "/objects");
> >  }
> >  
> > +Object *object_get_internal_root(void)
> > +{
> > +    return container_get(object_get_root(), "/internal-objects");
> 
> Whatever you expose in the QOM tree is no longer internal. Other name?

I guess the key concept trying to be expressed is that this will hold
objects created internally by QEMU code, in response to other configuration
input, as distinct from objects created externally by the user via -object.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Stefan Hajnoczi]

On Mon, Sep 25, 2017 at 11:34:58AM +0300, Manos Pitsidianakis wrote:
> On Mon, Sep 25, 2017 at 04:14:26PM +0800, Peter Xu wrote:
> > On Mon, Sep 25, 2017 at 09:14:21AM +0200, Andreas Färber wrote:
> > > Am 25.09.2017 um 08:37 schrieb Peter Xu:
> > > > We have object_get_objects_root() to keep user created objects, however
> > > > no place for objects that will be used internally.  Create such a
> > > > container for internal objects.
> > > >
> > > > CC: Andreas Färber <afaerber@suse.de>
> > > > CC: Markus Armbruster <armbru@redhat.com>
> > > > CC: Paolo Bonzini <pbonzini@redhat.com>
> > > > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  include/qom/object.h | 10 ++++++++++
> > > >  qom/object.c         |  5 +++++
> > > >  2 files changed, 15 insertions(+)
> > > >
> > > > diff --git a/include/qom/object.h b/include/qom/object.h
> > > > index f3e5cff..f567052 100644
> > > > --- a/include/qom/object.h
> > > > +++ b/include/qom/object.h
> > > > @@ -1214,6 +1214,16 @@ Object *object_get_root(void);
> > > >  Object *object_get_objects_root(void);
> > > >
> > > >  /**
> > > > + * object_get_internal_root:
> > > > + *
> > > > + * Get the container object that holds internally used object
> > > > + * instances. This is the object at path "/internal-objects"
> > > > + *
> > > > + * Returns: the internal object container
> > > > + */
> > > > +Object *object_get_internal_root(void);
> > > > +
> > > > +/**
> > > >   * object_get_canonical_path_component:
> > > >   *
> > > >   * Returns: The final component in the object's canonical path.  The canonical
> > > > diff --git a/qom/object.c b/qom/object.c
> > > > index 3e18537..857cee7 100644
> > > > --- a/qom/object.c
> > > > +++ b/qom/object.c
> > > > @@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
> > > >      return container_get(object_get_root(), "/objects");
> > > >  }
> > > >
> > > > +Object *object_get_internal_root(void)
> > > > +{
> > > > +    return container_get(object_get_root(), "/internal-objects");
> > > 
> > > Whatever you expose in the QOM tree is no longer internal. Other name?
> > 
> > Hi, Andreas,
> > 
> > If you mean "info qom-tree" here, can we still treat it as internal?
> > Since after all it's not exposed in QMP (while IMHO QMP is the
> > official protocol for QEMU clients).  And IIUC some HMP commands do
> > dump some internal structs for debugging purpose (like: "info
> > ramblock").
> > 
> > Or, do you have any suggestion?
> > 
> > (I did think about something like "hidden-objects", but I believe they
> > are not hidden as well if we think they are not internal...)
> > 
> > Thanks,
> > 
> > > 
> > > Regards,
> > > Andreas
> 
> If there's no need for internal IOThreads to be together with user created
> ones, a simple queue will be enough (include/qemu/queue.h).

Two solutions:

1. Manos' suggestion is Object->parent == NULL.  Simply don't add the
   Object to a parent.

2. Create an internal root that isn't accessible via QMP.
   Object *internal_root = object_new("container).

Stefan

Re: [Qemu-devel] [PATCH v2 3/4] iothread: export iothread_stop()

[Stefan Hajnoczi]

On Mon, Sep 25, 2017 at 02:37:28PM +0800, Peter Xu wrote:
> So that internal iothread users can explicitly stop one iothread without
> destroying it.
> 
> Since at it, fix iothread_stop() to allow it to be called multiple
> times.  Before this patch we may call iothread_stop() more than once on
> single iothread, while that may not be correct since qemu_thread_join()
> is not allowed to run twice.  From manual of pthread_join():
> 
>   Joining with a thread that has previously been joined results in
>   undefined behavior.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/sysemu/iothread.h |  1 +
>  iothread.c                | 24 ++++++++++++++++--------
>  2 files changed, 17 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/4] iothread: delay the context release to finalize

[Stefan Hajnoczi]

On Mon, Sep 25, 2017 at 02:37:29PM +0800, Peter Xu wrote:
> When gcontext is used with iothread, the context will be destroyed
> during iothread_stop().  That's not good since sometimes we would like
> to keep the resources until iothread is destroyed, but we may want to
> stop the thread before that point.
> 
> Delay the destruction of gcontext to iothread finalize.  Then we can do:
> 
>   iothread_stop(thread);
>   some_cleanup_on_resources();
>   iothread_destroy(thread);
> 
> We may need this patch if we want to run chardev IOs in iothreads and
> hopefully clean them up correctly.  For more specific information,
> please see 2b316774f6 ("qemu-char: do not operate on sources from
> finalize callbacks").
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  iothread.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/4] qom: provide root container for internal objs

[Peter Xu]

On Mon, Sep 25, 2017 at 11:00:33AM +0200, Andreas Färber wrote:
> Hi,
> 
> Am 25.09.2017 um 10:14 schrieb Peter Xu:
> > On Mon, Sep 25, 2017 at 09:14:21AM +0200, Andreas Färber wrote:
> >> Am 25.09.2017 um 08:37 schrieb Peter Xu:
> >>> We have object_get_objects_root() to keep user created objects, however
> >>> no place for objects that will be used internally.  Create such a
> >>> container for internal objects.
> >>>
> >>> CC: Andreas Färber <afaerber@suse.de>
> >>> CC: Markus Armbruster <armbru@redhat.com>
> >>> CC: Paolo Bonzini <pbonzini@redhat.com>
> >>> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> >>> Signed-off-by: Peter Xu <peterx@redhat.com>
> >>> ---
> >>>  include/qom/object.h | 10 ++++++++++
> >>>  qom/object.c         |  5 +++++
> >>>  2 files changed, 15 insertions(+)
> >>>
> >>> diff --git a/include/qom/object.h b/include/qom/object.h
> >>> index f3e5cff..f567052 100644
> >>> --- a/include/qom/object.h
> >>> +++ b/include/qom/object.h
> >>> @@ -1214,6 +1214,16 @@ Object *object_get_root(void);
> >>>  Object *object_get_objects_root(void);
> >>>  
> >>>  /**
> >>> + * object_get_internal_root:
> >>> + *
> >>> + * Get the container object that holds internally used object
> >>> + * instances. This is the object at path "/internal-objects"
> >>> + *
> >>> + * Returns: the internal object container
> >>> + */
> >>> +Object *object_get_internal_root(void);
> >>> +
> >>> +/**
> >>>   * object_get_canonical_path_component:
> >>>   *
> >>>   * Returns: The final component in the object's canonical path.  The canonical
> >>> diff --git a/qom/object.c b/qom/object.c
> >>> index 3e18537..857cee7 100644
> >>> --- a/qom/object.c
> >>> +++ b/qom/object.c
> >>> @@ -1370,6 +1370,11 @@ Object *object_get_objects_root(void)
> >>>      return container_get(object_get_root(), "/objects");
> >>>  }
> >>>  
> >>> +Object *object_get_internal_root(void)
> >>> +{
> >>> +    return container_get(object_get_root(), "/internal-objects");
> >>
> >> Whatever you expose in the QOM tree is no longer internal. Other name?
> > 
> > Hi, Andreas,
> > 
> > If you mean "info qom-tree" here, can we still treat it as internal?
> > Since after all it's not exposed in QMP (while IMHO QMP is the
> > official protocol for QEMU clients).  And IIUC some HMP commands do
> > dump some internal structs for debugging purpose (like: "info
> > ramblock").
> > 
> > Or, do you have any suggestion?
> > 
> > (I did think about something like "hidden-objects", but I believe they
> >  are not hidden as well if we think they are not internal...)
> 
> I'm on travels and only seeing this 1/4 without context - according to
> Manos apparently something about IOThreads.
> 
> The reason that certain container groups such as "/machine/peripheral"
> exist was more of a legacy reason for migration from qdev, so yes I am
> critical of "/hidden-objects" as well. If the objects are not related to
> an existing device, you could just place them somewhere outside
> "/machine", e.g. directly in the root node or in a container specific to
> that use case (/io? /threads?), rather than a new generic bucket of
> whatever name. Any objects not under /machine should be close to what
> you consider internal.
> 
> Note that "info qom-tree" is not the only way to query the objects,
> there's some Python QMP scripts as well.

I see.  I'll take away Stefan's suggestion to create a standalone
container for internal objects, so that it will be totally hidden.

I didn't choose to allow parent == NULL to avoid breaking or modifying
QOM interfaces.

Preparing another post.  Thanks all!

> 
> Regards,
> Andreas
> 
> -- 
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

-- 
Peter Xu