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

[Fam Zheng <famz@redhat.com>]

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