[PATCH] iothread: Set the GSource "name" field

[PATCH] iothread: Set the GSource "name" field

[Fabiano Rosas]

Having a name in the source helps with debugging core dumps when one
might not have access to TLS data to cross-reference AioContexts with
their addresses.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 iothread.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/iothread.c b/iothread.c
index b41c305bd9..8cfb9d5be6 100644
--- a/iothread.c
+++ b/iothread.c
@@ -138,15 +138,19 @@ static void iothread_instance_finalize(Object *obj)
     qemu_sem_destroy(&iothread->init_done_sem);
 }
 
-static void iothread_init_gcontext(IOThread *iothread)
+static void iothread_init_gcontext(IOThread *iothread, char *thread_name)
 {
     GSource *source;
+    gchar *name = g_strdup_printf("%s aio-context", thread_name);
 
     iothread->worker_context = g_main_context_new();
     source = aio_get_g_source(iothread_get_aio_context(iothread));
+    g_source_set_name(source, name);
     g_source_attach(source, iothread->worker_context);
     g_source_unref(source);
     iothread->main_loop = g_main_loop_new(iothread->worker_context, TRUE);
+
+    g_free(name);
 }
 
 static void iothread_set_aio_context_params(EventLoopBase *base, Error **errp)
@@ -189,11 +193,14 @@ static void iothread_init(EventLoopBase *base, Error **errp)
         return;
     }
 
+    thread_name = g_strdup_printf("IO %s",
+                        object_get_canonical_path_component(OBJECT(base)));
+
     /*
      * Init one GMainContext for the iothread unconditionally, even if
      * it's not used
      */
-    iothread_init_gcontext(iothread);
+    iothread_init_gcontext(iothread, thread_name);
 
     iothread_set_aio_context_params(base, &local_error);
     if (local_error) {
@@ -206,8 +213,6 @@ static void iothread_init(EventLoopBase *base, Error **errp)
     /* This assumes we are called from a thread with useful CPU affinity for us
      * to inherit.
      */
-    thread_name = g_strdup_printf("IO %s",
-                        object_get_canonical_path_component(OBJECT(base)));
     qemu_thread_create(&iothread->thread, thread_name, iothread_run,
                        iothread, QEMU_THREAD_JOINABLE);
     g_free(thread_name);
-- 
2.35.3

Re: [PATCH] iothread: Set the GSource "name" field

[Philippe Mathieu-Daudé]

On 4/9/23 16:48, Fabiano Rosas wrote:
> Having a name in the source helps with debugging core dumps when one
> might not have access to TLS data to cross-reference AioContexts with
> their addresses.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   iothread.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] iothread: Set the GSource "name" field

[Peter Xu]

On Mon, Sep 04, 2023 at 11:48:11AM -0300, Fabiano Rosas wrote:
> @@ -189,11 +193,14 @@ static void iothread_init(EventLoopBase *base, Error **errp)
>          return;
>      }
>  
> +    thread_name = g_strdup_printf("IO %s",
> +                        object_get_canonical_path_component(OBJECT(base)));
> +
>      /*
>       * Init one GMainContext for the iothread unconditionally, even if
>       * it's not used
>       */
> -    iothread_init_gcontext(iothread);
> +    iothread_init_gcontext(iothread, thread_name);
>  
>      iothread_set_aio_context_params(base, &local_error);
>      if (local_error) {

I think thread_name might be leaked if error here.  Thanks,

-- 
Peter Xu

Re: [PATCH] iothread: Set the GSource "name" field

[Philippe Mathieu-Daudé]

On 5/9/23 17:45, Peter Xu wrote:
> On Mon, Sep 04, 2023 at 11:48:11AM -0300, Fabiano Rosas wrote:
>> @@ -189,11 +193,14 @@ static void iothread_init(EventLoopBase *base, Error **errp)
>>           return;
>>       }
>>   
>> +    thread_name = g_strdup_printf("IO %s",
>> +                        object_get_canonical_path_component(OBJECT(base)));
>> +
>>       /*
>>        * Init one GMainContext for the iothread unconditionally, even if
>>        * it's not used
>>        */
>> -    iothread_init_gcontext(iothread);
>> +    iothread_init_gcontext(iothread, thread_name);
>>   
>>       iothread_set_aio_context_params(base, &local_error);
>>       if (local_error) {
> 
> I think thread_name might be leaked if error here.  Thanks,

Oops, good catch. Better switch to g_autofree.

Re: [PATCH] iothread: Set the GSource "name" field

[Stefan Hajnoczi]

On Tue, 5 Sept 2023 at 12:51, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 5/9/23 17:45, Peter Xu wrote:
> > On Mon, Sep 04, 2023 at 11:48:11AM -0300, Fabiano Rosas wrote:
> >> @@ -189,11 +193,14 @@ static void iothread_init(EventLoopBase *base, Error **errp)
> >>           return;
> >>       }
> >>
> >> +    thread_name = g_strdup_printf("IO %s",
> >> +                        object_get_canonical_path_component(OBJECT(base)));
> >> +
> >>       /*
> >>        * Init one GMainContext for the iothread unconditionally, even if
> >>        * it's not used
> >>        */
> >> -    iothread_init_gcontext(iothread);
> >> +    iothread_init_gcontext(iothread, thread_name);
> >>
> >>       iothread_set_aio_context_params(base, &local_error);
> >>       if (local_error) {
> >
> > I think thread_name might be leaked if error here.  Thanks,
>
> Oops, good catch. Better switch to g_autofree.

Yes, please.

And also make the iothread_init_gcontext(..., char *thread_name)
argument const char * to indicate that the name is owned by the caller
and not modified by iothread_init_gcontext().

Thanks,
Stefan