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

2011-08-11 Thread Maarten Bosmans
2011/8/11 Colin Guthrie gm...@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...@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@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss


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

2011-08-10 Thread Lu Guanqun
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...@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) {

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss