Re: [PATCH v3 04/20] util: expose qemu_thread_set_name

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Sep 19, 2025 at 10:39:31AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > The ability to set the thread name needs to be used in a number
> > of places, so expose the current impls as public methods.
> >
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  include/qemu/thread.h    |  1 +
> >  util/qemu-thread-posix.c | 30 ++++++++++++++++++------------
> >  util/qemu-thread-win32.c |  6 +++---
> >  3 files changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> > index 3a286bb3ef..27b888ab0a 100644
> > --- a/include/qemu/thread.h
> > +++ b/include/qemu/thread.h
> > @@ -215,6 +215,7 @@ void *qemu_thread_join(QemuThread *thread);
> >  void qemu_thread_get_self(QemuThread *thread);
> >  bool qemu_thread_is_self(QemuThread *thread);
> >  G_NORETURN void qemu_thread_exit(void *retval);
> > +void qemu_thread_set_name(const char *name);
> >  
> >  struct Notifier;
> >  /**
> > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> > index 7c985b5d38..ddaa1de4dd 100644
> > --- a/util/qemu-thread-posix.c
> > +++ b/util/qemu-thread-posix.c
> > @@ -329,6 +329,21 @@ static void qemu_thread_atexit_notify(void *arg)
> >      notifier_list_notify(&thread_exit, NULL);
> >  }
> >  
> > +void qemu_thread_set_name(const char *name)
> > +{
> > +    /*
> > +     * Attempt to set the threads name; note that this is for debug, so
> > +     * we're not going to fail if we can't set it.
> > +     */
> > +# if defined(CONFIG_PTHREAD_SETNAME_NP_W_TID)
> > +    pthread_setname_np(pthread_self(), name);
> > +# elif defined(CONFIG_PTHREAD_SETNAME_NP_WO_TID)
> > +    pthread_setname_np(name);
> > +# elif defined(CONFIG_PTHREAD_SET_NAME_NP)
> > +    pthread_set_name_np(pthread_self(), name);
> > +# endif
> > +}
> > +
> >  typedef struct {
> >      void *(*start_routine)(void *);
> >      void *arg;
> > @@ -342,20 +357,11 @@ static void *qemu_thread_start(void *args)
> >      void *arg = qemu_thread_args->arg;
> >      void *r;
> >  
> > -    /* Attempt to set the threads name; note that this is for debug, so
> > -     * we're not going to fail if we can't set it.
> > -     */
> >      if (qemu_thread_args->name) {
> > -# if defined(CONFIG_PTHREAD_SETNAME_NP_W_TID)
> > -        pthread_setname_np(pthread_self(), qemu_thread_args->name);
> > -# elif defined(CONFIG_PTHREAD_SETNAME_NP_WO_TID)
> > -        pthread_setname_np(qemu_thread_args->name);
> > -# elif defined(CONFIG_PTHREAD_SET_NAME_NP)
> > -        pthread_set_name_np(pthread_self(), qemu_thread_args->name);
> > -# endif
> > +        qemu_thread_set_name(qemu_thread_args->name);
> 
> Clean factoring out up to here.
> 
> > +        QEMU_TSAN_ANNOTATE_THREAD_NAME(qemu_thread_args->name);
> > +        g_free(qemu_thread_args->name);
> >      }
> > -    QEMU_TSAN_ANNOTATE_THREAD_NAME(qemu_thread_args->name);
> > -    g_free(qemu_thread_args->name);
> 
> This part avoids
> 
>        QEMU_TSAN_ANNOTATE_THREAD_NAME(NULL);
>        g_free(NULL);
> 
> The latter is an obvious no-op.  The former isn't.  Separate patch or
> mention in the commit message?

I'll just drop this since it isn't needed by the patch

> 
> >      g_free(qemu_thread_args);
> >  
> >      /*
> > diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> > index 9595a5b090..62eaa11026 100644
> > --- a/util/qemu-thread-win32.c
> > +++ b/util/qemu-thread-win32.c
> > @@ -316,7 +316,7 @@ void *qemu_thread_join(QemuThread *thread)
> >      return ret;
> >  }
> >  
> > -static void set_thread_description(HANDLE h, const char *name)
> > +void qemu_thread_set_name(const char *name)
> >  {
> >      g_autofree wchar_t *namew = NULL;
> >  
> > @@ -329,7 +329,7 @@ static void set_thread_description(HANDLE h, const char *name)
> >          return;
> >      }
> >  
> > -    SetThreadDescriptionFunc(h, namew);
> > +    SetThreadDescriptionFunc(GetCurrentThread(), namew);
> >  }
> >  
> >  void qemu_thread_create(QemuThread *thread, const char *name,
> > @@ -356,7 +356,7 @@ void qemu_thread_create(QemuThread *thread, const char *name,
>        hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
>                                          data, 0, &thread->tid);
>        if (!hThread) {
> >          error_exit(GetLastError(), __func__);
> >      }
> >      if (name) {
> > -        set_thread_description(hThread, name);
> > +        qemu_thread_set_name(name);
> >      }
> >      CloseHandle(hThread);
> 
> This changes the first argument of SetThreadDescriptionFunc() from the
> value of _beginthreadex() to GetCurrentThread().  I'm a happy Windows
> ignoramus...  sure these refer to the same thread?

No, I messed up here. I should have moved the qemu_thread_set_name call
into the thread function, as this is still the parent thread.

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