Re: [PATCH v5 05/24] util: expose qemu_thread_set_name

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

On Tue, Jan 13, 2026 at 10:16:06AM +0100, 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>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  include/qemu/thread.h    |  1 +
> >  util/qemu-thread-posix.c | 26 ++++++++++++++++----------
> >  util/qemu-thread-win32.c | 13 ++++++++-----
> >  3 files changed, 25 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..b1c127dbe3 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,17 +357,8 @@ 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);
> >      }
> >      QEMU_TSAN_ANNOTATE_THREAD_NAME(qemu_thread_args->name);
> >      g_free(qemu_thread_args->name);
> 
> qemu_thread_set_name() is factored out.  No change in behavior.
> 
> > diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> > index 9595a5b090..4d2d663a9a 100644
> > --- a/util/qemu-thread-win32.c
> > +++ b/util/qemu-thread-win32.c
> > @@ -225,6 +225,7 @@ struct QemuThreadData {
> >      void             *arg;
> >      short             mode;
> >      NotifierList      exit;
> > +    char             *name;
> >  
> >      /* Only used for joinable threads. */
> >      bool              exited;
> > @@ -266,6 +267,10 @@ static unsigned __stdcall win32_start_routine(void *arg)
> >      void *(*start_routine)(void *) = data->start_routine;
> >      void *thread_arg = data->arg;
> >  
> > +    if (data->name) {
> > +        qemu_thread_set_name(data->name);
> > +        g_clear_pointer(&data->name, g_free);
> > +    }
> >      qemu_thread_data = data;
> >      qemu_thread_exit(start_routine(thread_arg));
> >      abort();
> > @@ -316,7 +321,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 +334,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,
> > @@ -344,6 +349,7 @@ void qemu_thread_create(QemuThread *thread, const char *name,
> >      data->arg = arg;
> >      data->mode = mode;
> >      data->exited = false;
> > +    data->name = g_strdup(name);
> >      notifier_list_init(&data->exit);
> >  
> >      if (data->mode != QEMU_THREAD_DETACHED) {
> > @@ -355,9 +361,6 @@ void qemu_thread_create(QemuThread *thread, const char *name,
> >      if (!hThread) {
> >          error_exit(GetLastError(), __func__);
> >      }
> > -    if (name) {
> > -        set_thread_description(hThread, name);
> > -    }
> >      CloseHandle(hThread);
> >  
> >      thread->data = data;
> 
> This delays setting the thread name until the thread runs.  Fine, I
> guess, but I'd mention it in the commit message.

Returning to this, the change is arguably /not/ delaying setting the
thread name, but in fact fixing a race condition when settnig the
thread name.

This existing 'set_thread_description' call is taking place in the
parent thread, and at this point in the time the child thread has
potentially already been scheduled to run.  So if the parent thread
was unscheduled, it is possible the child was running without having
it name set. This change eliminates that race.

This distinction is important enough that I think I'll move this
specific change out into its own self-contained commit.

> 
> The new QemuThreadData member @name is non-null only between thread
> creation and thread name setting, unlike the other members under
> /* Passed to win32_start_routine.  */  Worth a comment?  Keep it alive
> until the thread dies?  Entirely up to you.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

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