On Tue, Apr 10, 2018 at 08:35:40AM -0500, Eric Blake wrote: > On 04/10/2018 07:49 AM, Peter Xu wrote: > > We will conditionally have a wrapper layer depending on whether the host > > has the PTHREAD_SETNAME capability. It complicates stuff. Let's just > > keep the wrapper there, meanwhile we opt out the pthread_setname_np() > > call only. The layer can be helpful in future patches to pass data from > > the parent thread to the child thread. > > > > Signed-off-by: Peter Xu <pet...@redhat.com> > > --- > > util/qemu-thread-posix.c | 33 +++++++++++++-------------------- > > 1 file changed, 13 insertions(+), 20 deletions(-) > > > > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c > > index b789cf32e9..3ae96210d6 100644 > > --- a/util/qemu-thread-posix.c > > +++ b/util/qemu-thread-posix.c > > @@ -482,7 +482,6 @@ static void __attribute__((constructor)) > > qemu_thread_atexit_init(void) > > } > > > > More context: > > static bool name_threads; > > void qemu_thread_naming(bool enable) > { > name_threads = enable; > > #ifndef CONFIG_THREAD_SETNAME_BYTHREAD > /* This is a debugging option, not fatal */ > if (enable) { > fprintf(stderr, "qemu: thread naming not supported on this host\n"); > } > #endif > } > > > > > > -#ifdef CONFIG_PTHREAD_SETNAME_NP > > Why are we using CONFIG_THREAD_SETNAME_BYTHREAD in one place, and > CONFIG_PTHREAD_SETNAME_NP in another? > > /me checks configure - oh: > > # Hold two types of flag: > # CONFIG_THREAD_SETNAME_BYTHREAD - we've got a way of setting the name on > # a thread we have a handle to > # CONFIG_PTHREAD_SETNAME_NP - A way of doing it on a particular > # platform > > even though, right now, we only either set both flags at once or leave > both clear, since we don't (yet?) have any other platform-specific ways > to do it.
It seems so. I'm not sure whether they could be useful in the future and I'm fine with them, hence I keep it as is. > > > typedef struct { > > void *(*start_routine)(void *); > > void *arg; > > @@ -498,13 +497,15 @@ static void *qemu_thread_start(void *args) > > /* 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. > > */ > > - pthread_setname_np(pthread_self(), qemu_thread_args->name); > > +#ifdef CONFIG_PTHREAD_SETNAME_NP > > + if (qemu_thread_args->name) { > > + pthread_setname_np(pthread_self(), qemu_thread_args->name); > > Post-patch, this (attempts to) set the thread name if a non-NULL name is > present... > > > > > > -#ifdef CONFIG_PTHREAD_SETNAME_NP > > - if (name_threads) { > > - QemuThreadArgs *qemu_thread_args; > > - qemu_thread_args = g_new0(QemuThreadArgs, 1); > > - qemu_thread_args->name = g_strdup(name); > > ...but pre-patch, qemu_thread_args->name was left NULL unless > name_threads was true, because someone had called > qemu_thread_naming(true)... > > > - qemu_thread_args->start_routine = start_routine; > > - qemu_thread_args->arg = arg; > > - > > - err = pthread_create(&thread->thread, &attr, > > - qemu_thread_start, qemu_thread_args); > > - } else > > -#endif > > - { > > - err = pthread_create(&thread->thread, &attr, > > - start_routine, arg); > > - } > > + qemu_thread_args = g_new0(QemuThreadArgs, 1); > > + qemu_thread_args->name = g_strdup(name); > > ...so you have changed semantics - you are now unconditionally trying to > set the thread name, instead of honoring qemu_thread_naming(). Do we > still need qemu_thread_naming() (tied to opt debug-threads)? > > You need to either fix your code to remain conditional on whether > name_threads is set, or document the semantic change as intentional in > the commit message. Indeed, thanks for catching that. What I really wanted is probably this: static void *qemu_thread_start(void *args) { ... #ifdef CONFIG_PTHREAD_SETNAME_NP if (name_threads && qemu_thread_args->name) { pthread_setname_np(pthread_self(), qemu_thread_args->name); } #endif ... } I'll fix that up in next version. > > However, the idea for refactoring to always use the shim makes sense. Thanks, -- Peter Xu