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

Since at it, fix iothread_stop() to allow re-entrance.  Before this
patch we may call iothread_stop() twice 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 74e400c..894756b 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->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 || !iothread->ctx) {
+        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

On Fri, 09/22 16:56, Peter Xu wrote:
> So that internal iothread users can explicitly stop one iothread without
> destroying it.
> 
> Since at it, fix iothread_stop() to allow re-entrance.  Before this

I don't think there is any re-entrace here. Maybe you mean

s/re-entrance/calling multiple times/

?

> patch we may call iothread_stop() twice on single iothread, while that
> may not be correct since qemu_thread_join() is not allowed to run twice.
> From manual of pthread_join():

Is one call from iothread_stop_all() and one from object finalize?

> 
>   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 74e400c..894756b 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->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 || !iothread->ctx) {
> +        return 0;
> +    }

I think the check of iothread->ctx can be moved to iothread_stop() too.

Fam


> +    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
> 
> 

On Fri, Sep 22, 2017 at 09:06:26PM +0800, Fam Zheng wrote:
> On Fri, 09/22 16:56, Peter Xu wrote:
> > So that internal iothread users can explicitly stop one iothread without
> > destroying it.
> > 
> > Since at it, fix iothread_stop() to allow re-entrance.  Before this
> 
> I don't think there is any re-entrace here. Maybe you mean
> 
> s/re-entrance/calling multiple times/
> 
> ?

Yes, you are right.

> 
> > patch we may call iothread_stop() twice on single iothread, while that
> > may not be correct since qemu_thread_join() is not allowed to run twice.
> > From manual of pthread_join():
> 
> Is one call from iothread_stop_all() and one from object finalize?

Yes.

> 
> > 
> >   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 74e400c..894756b 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->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 || !iothread->ctx) {
> > +        return 0;
> > +    }
> 
> I think the check of iothread->ctx can be moved to iothread_stop() too.

Yes, will do.

-- 
Peter Xu