[pulseaudio-discuss] [PATCH v2] thread-posix: remove duplicate code for setting thread name

[Maarten Bosmans]

2011/8/11 Colin Guthrie <gmane at colin.guthr.ie>:
> 'Twas brillig, and Lu Guanqun at 11/08/11 10:54 did gyre and gimble:
>> On Thu, Aug 11, 2011 at 04:30:12PM +0800, Colin Guthrie wrote:
>>> 'Twas brillig, and Lu Guanqun at 11/08/11 02:59 did gyre and gimble:
>>>> According to the principle of DRY (don't repeat yourself), remove the code for
>>>> setting thread name in thread-posix.c.
>>>>
>>>> Signed-off-by: Lu Guanqun <guanqun.lu at intel.com>
>>>> ---
>>>>  src/pulsecore/thread-posix.c |   20 ++++++++++----------
>>>>  1 files changed, 10 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/src/pulsecore/thread-posix.c b/src/pulsecore/thread-posix.c
>>>> index 3f4ae5c..9a8c51b 100644
>>>> --- a/src/pulsecore/thread-posix.c
>>>> +++ b/src/pulsecore/thread-posix.c
>>>> @@ -65,15 +65,19 @@ static void thread_free_cb(void *p) {
>>>>
>>>>  PA_STATIC_TLS_DECLARE(current_thread, thread_free_cb);
>>>>
>>>> +static void set_thread_name(const char *name) {
>>>> +#ifdef __linux__
>>>> +    prctl(PR_SET_NAME, name);
>>>> +#elif defined(HAVE_PTHREAD_SETNAME_NP) && defined(OS_IS_DARWIN)
>>>> +    pthread_setname_np(name);
>>>> +#endif
>>>> +}
>>>> +
>>>>  static void* internal_thread_func(void *userdata) {
>>>>      pa_thread *t = userdata;
>>>>      pa_assert(t);
>>>>
>>>> -#ifdef __linux__
>>>> -    prctl(PR_SET_NAME, t->name);
>>>> -#elif defined(HAVE_PTHREAD_SETNAME_NP) && defined(OS_IS_DARWIN)
>>>> -    pthread_setname_np(t->name);
>>>> -#endif
>>>> +    set_thread_name(t->name);
>>>>
>>>>      t->id = pthread_self();
>>>>
>>>> @@ -175,11 +179,7 @@ void pa_thread_set_name(pa_thread *t, const char *name) {
>>>>      pa_xfree(t->name);
>>>>      t->name = pa_xstrdup(name);
>>>>
>>>> -#ifdef __linux__
>>>> -    prctl(PR_SET_NAME, name);
>>>> -#elif defined(HAVE_PTHREAD_SETNAME_NP) && defined(OS_IS_DARWIN)
>>>> -    pthread_setname_np(name);
>>>> -#endif
>>>> +    set_thread_name(name);
>>>>  }
>>>>
>>>>  const char *pa_thread_get_name(pa_thread *t) {
>>>
>>>
>>> Am I blind or is pa_thread_set_name() itself redundant? I cannot find
>>> any calls to it....
>>
>> Good catch! I overlooked the code, I thought it was called in
>> pa_thread_new(). Do we need to add it in pa_thread_new() btw?

Well, it looks like it is being set in internal_thread_func, so no
need to first set it in pa_thread_new. Also it looks like the function
that is used [prctl(PR_SET_NAME, name)] should be called from the
running thread itself, so internal_thread_func instead of
pa_thread_new is obviously the place to do it.

That leads me to suspect the whole code in pa_thread_set_name. That
seems to alter the name of the thread invoking the function, instead
of the thread pointed to by the first argument.

Maarten

> I have no idea..., but it does seem like it's missing...
>
> Also there is a memory leak in the case when pthread_create() fails
> (t->name is not freed).
>
>
> Anyone have any specific info here?
>
> Col
>
>
>
>
>
> --
>
> Colin Guthrie
> gmane(at)colin.guthr.ie
> http://colin.guthr.ie/
>
> Day Job:
>  Tribalogic Limited [http://www.tribalogic.net/]
> Open Source:
>  Mageia Contributor [http://www.mageia.org/]
>  PulseAudio Hacker [http://www.pulseaudio.org/]
>  Trac Hacker [http://trac.edgewall.org/]
>
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
>