[lxc-devel] [PATCH] valgrind drd tool shows conflicting stores happening at lxc_global_config_value@src/lxc/utils.c (v2)

2013-11-01 Thread S . Çağlar Onur 
Conflict occurs between following lines

[...]
269 if (values[i])
270 return values[i];
[...]

and

[...]
309 /* could not find value, use default */
310 values[i] = (*ptr)[1];
[...]

fix it using a specific lock dedicated to that problem as Serge suggested.

Also introduce a new autoconf parameter (--enable-mutex-debugging) to convert 
mutexes to error reporting type and to provide a stacktrace when locking fails.

Signed-off-by: S.Çağlar Onur cag...@10ur.org
---
 configure.ac  |  9 ++
 src/lxc/cgroup.c  |  2 +-
 src/lxc/lxclock.c | 17 +--
 src/lxc/start.c   |  2 +-
 src/lxc/utils.c   | 90 ---
 src/lxc/utils.h   |  5 +++-
 6 files changed, 115 insertions(+), 10 deletions(-)

diff --git a/configure.ac b/configure.ac
index 9fedf55..6004b35 100644
--- a/configure.ac
+++ b/configure.ac
@@ -178,6 +178,15 @@ AM_COND_IF([ENABLE_PYTHON],
PKG_CHECK_MODULES([PYTHONDEV], [python3 = 3.2],[],[AC_MSG_ERROR([You 
must install python3-dev])])
AC_DEFINE_UNQUOTED([ENABLE_PYTHON], 1, [Python3 is available])])
 
+# Enable dumping stack traces
+AC_ARG_ENABLE([mutex-debugging],
+   [AC_HELP_STRING([--enable-mutex-debugging], [Makes mutexes to report 
error and provide stack trace])],
+   [enable_mutex_debugging=yes], [enable_mutex_debugging=no])
+AM_CONDITIONAL([MUTEX_DEBUGGING], [test x$enable_mutex_debugging = xyes])
+
+AM_COND_IF([MUTEX_DEBUGGING],
+   AC_DEFINE_UNQUOTED([MUTEX_DEBUGGING], 1, [Enabling mutex debugging]))
+
 # Not in older autoconf versions
 # AS_VAR_COPY(DEST, SOURCE)
 # -
diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index 01ed040..1e1e72a 100644
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -91,7 +91,7 @@ struct cgroup_meta_data *lxc_cgroup_load_meta()
int saved_errno;
 
errno = 0;
-   cgroup_use = lxc_global_config_value(cgroup.use);
+   cgroup_use = default_cgroup_use();
if (!cgroup_use  errno != 0)
return NULL;
if (cgroup_use) {
diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
index d403bcc..3857ff0 100644
--- a/src/lxc/lxclock.c
+++ b/src/lxc/lxclock.c
@@ -18,15 +18,15 @@
  *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  
USA
  */
 
-#include pthread.h
+#define _GNU_SOURCE
 #include lxclock.h
 #include malloc.h
 #include stdio.h
 #include errno.h
 #include unistd.h
 #include fcntl.h
-#define _GNU_SOURCE
 #include stdlib.h
+#include pthread.h
 #include lxc/utils.h
 #include lxc/log.h
 #include lxc/lxccontainer.h
@@ -38,7 +38,11 @@
 
 lxc_log_define(lxc_lock, lxc);
 
+#ifdef MUTEX_DEBUGGING
+pthread_mutex_t thread_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+#else
 pthread_mutex_t thread_mutex = PTHREAD_MUTEX_INITIALIZER;
+#endif
 
 static char *lxclock_name(const char *p, const char *n)
 {
@@ -267,13 +271,20 @@ void process_lock(void)
 
if ((ret = pthread_mutex_lock(thread_mutex)) != 0) {
ERROR(pthread_mutex_lock returned:%d %s, ret, strerror(ret));
+   dump_stacktrace();
exit(1);
}
 }
 
 void process_unlock(void)
 {
-   pthread_mutex_unlock(thread_mutex);
+   int ret;
+
+   if ((ret = pthread_mutex_unlock(thread_mutex)) != 0) {
+   ERROR(pthread_mutex_unlock returned:%d %s, ret, 
strerror(ret));
+   dump_stacktrace();
+   exit(1);
+   }
 }
 
 int container_mem_lock(struct lxc_container *c)
diff --git a/src/lxc/start.c b/src/lxc/start.c
index 1cadc09..58e1194 100644
--- a/src/lxc/start.c
+++ b/src/lxc/start.c
@@ -695,7 +695,7 @@ int lxc_spawn(struct lxc_handler *handler)
 * default value is available
 */
if (getuid() == 0)
-   cgroup_pattern = lxc_global_config_value(cgroup.pattern);
+   cgroup_pattern = default_cgroup_pattern();
if (!cgroup_pattern)
cgroup_pattern = %n;
 
diff --git a/src/lxc/utils.c b/src/lxc/utils.c
index 9e2e326..590482e 100644
--- a/src/lxc/utils.c
+++ b/src/lxc/utils.c
@@ -21,7 +21,8 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
-#define _GNU_SOURCE
+#include config.h
+
 #include errno.h
 #include unistd.h
 #include stdlib.h
@@ -38,6 +39,8 @@
 #include sys/types.h
 #include sys/wait.h
 #include assert.h
+#include pthread.h
+#include execinfo.h
 
 #ifndef HAVE_GETLINE
 #ifdef HAVE_FGETLN
@@ -49,8 +52,61 @@
 #include log.h
 #include lxclock.h
 
+#define MAX_STACKDEPTH 25
+
 lxc_log_define(lxc_utils, lxc);
 
+
+#ifdef MUTEX_DEBUGGING
+static pthread_mutex_t static_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+
+inline void dump_stacktrace(void)
+{
+   void *array[MAX_STACKDEPTH];
+   size_t size;
+   char **strings;
+   size_t i;
+
+   size = backtrace(array, MAX_STACKDEPTH);
+   strings = backtrace_symbols(array, size);
+
+   // Using fprintf here as our logging module

Re: [lxc-devel] [PATCH] valgrind drd tool shows conflicting stores happening at lxc_global_config_value@src/lxc/utils.c (v2)

2013-11-01 Thread Serge Hallyn 
Quoting S.Çağlar Onur (cag...@10ur.org):
 Conflict occurs between following lines
 
 [...]
 269 if (values[i])
 270 return values[i];
 [...]
 
 and
 
 [...]
 309 /* could not find value, use default */
 310 values[i] = (*ptr)[1];
 [...]
 
 fix it using a specific lock dedicated to that problem as Serge suggested.
 
 Also introduce a new autoconf parameter (--enable-mutex-debugging) to convert 
 mutexes to error reporting type and to provide a stacktrace when locking 
 fails.
 
 Signed-off-by: S.Çağlar Onur cag...@10ur.org

Thanks.

Acked-by: Serge E. Hallyn serge.hal...@ubuntu.com

(Just a note - you appear to have expand-tab set in your editor,
as you're replacing some tabs with spaces in this patch.)

 ---
  configure.ac  |  9 ++
  src/lxc/cgroup.c  |  2 +-
  src/lxc/lxclock.c | 17 +--
  src/lxc/start.c   |  2 +-
  src/lxc/utils.c   | 90 
 ---
  src/lxc/utils.h   |  5 +++-
  6 files changed, 115 insertions(+), 10 deletions(-)
 
 diff --git a/configure.ac b/configure.ac
 index 9fedf55..6004b35 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -178,6 +178,15 @@ AM_COND_IF([ENABLE_PYTHON],
   PKG_CHECK_MODULES([PYTHONDEV], [python3 = 3.2],[],[AC_MSG_ERROR([You 
 must install python3-dev])])
   AC_DEFINE_UNQUOTED([ENABLE_PYTHON], 1, [Python3 is available])])
  
 +# Enable dumping stack traces
 +AC_ARG_ENABLE([mutex-debugging],
 + [AC_HELP_STRING([--enable-mutex-debugging], [Makes mutexes to report 
 error and provide stack trace])],
 + [enable_mutex_debugging=yes], [enable_mutex_debugging=no])
 +AM_CONDITIONAL([MUTEX_DEBUGGING], [test x$enable_mutex_debugging = xyes])
 +
 +AM_COND_IF([MUTEX_DEBUGGING],
 + AC_DEFINE_UNQUOTED([MUTEX_DEBUGGING], 1, [Enabling mutex debugging]))
 +
  # Not in older autoconf versions
  # AS_VAR_COPY(DEST, SOURCE)
  # -
 diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
 index 01ed040..1e1e72a 100644
 --- a/src/lxc/cgroup.c
 +++ b/src/lxc/cgroup.c
 @@ -91,7 +91,7 @@ struct cgroup_meta_data *lxc_cgroup_load_meta()
   int saved_errno;
  
   errno = 0;
 - cgroup_use = lxc_global_config_value(cgroup.use);
 +   cgroup_use = default_cgroup_use();
   if (!cgroup_use  errno != 0)
   return NULL;
   if (cgroup_use) {
 diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
 index d403bcc..3857ff0 100644
 --- a/src/lxc/lxclock.c
 +++ b/src/lxc/lxclock.c
 @@ -18,15 +18,15 @@
   *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  
 02110-1301  USA
   */
  
 -#include pthread.h
 +#define _GNU_SOURCE
  #include lxclock.h
  #include malloc.h
  #include stdio.h
  #include errno.h
  #include unistd.h
  #include fcntl.h
 -#define _GNU_SOURCE
  #include stdlib.h
 +#include pthread.h
  #include lxc/utils.h
  #include lxc/log.h
  #include lxc/lxccontainer.h
 @@ -38,7 +38,11 @@
  
  lxc_log_define(lxc_lock, lxc);
  
 +#ifdef MUTEX_DEBUGGING
 +pthread_mutex_t thread_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
 +#else
  pthread_mutex_t thread_mutex = PTHREAD_MUTEX_INITIALIZER;
 +#endif
  
  static char *lxclock_name(const char *p, const char *n)
  {
 @@ -267,13 +271,20 @@ void process_lock(void)
  
   if ((ret = pthread_mutex_lock(thread_mutex)) != 0) {
   ERROR(pthread_mutex_lock returned:%d %s, ret, strerror(ret));
 + dump_stacktrace();
   exit(1);
   }
  }
  
  void process_unlock(void)
  {
 - pthread_mutex_unlock(thread_mutex);
 + int ret;
 +
 + if ((ret = pthread_mutex_unlock(thread_mutex)) != 0) {
 + ERROR(pthread_mutex_unlock returned:%d %s, ret, 
 strerror(ret));
 + dump_stacktrace();
 + exit(1);
 + }
  }
  
  int container_mem_lock(struct lxc_container *c)
 diff --git a/src/lxc/start.c b/src/lxc/start.c
 index 1cadc09..58e1194 100644
 --- a/src/lxc/start.c
 +++ b/src/lxc/start.c
 @@ -695,7 +695,7 @@ int lxc_spawn(struct lxc_handler *handler)
* default value is available
*/
   if (getuid() == 0)
 - cgroup_pattern = lxc_global_config_value(cgroup.pattern);
 +   cgroup_pattern = default_cgroup_pattern();
   if (!cgroup_pattern)
   cgroup_pattern = %n;
  
 diff --git a/src/lxc/utils.c b/src/lxc/utils.c
 index 9e2e326..590482e 100644
 --- a/src/lxc/utils.c
 +++ b/src/lxc/utils.c
 @@ -21,7 +21,8 @@
   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
 USA
   */
  
 -#define _GNU_SOURCE
 +#include config.h
 +
  #include errno.h
  #include unistd.h
  #include stdlib.h
 @@ -38,6 +39,8 @@
  #include sys/types.h
  #include sys/wait.h
  #include assert.h
 +#include pthread.h
 +#include execinfo.h
  
  #ifndef HAVE_GETLINE
  #ifdef HAVE_FGETLN
 @@ -49,8 +52,61 @@
  #include log.h
  #include lxclock.h
  
 +#define MAX_STACKDEPTH 25
 +
  lxc_log_define(lxc_utils, lxc);
  
 +
 +#ifdef MUTEX_DEBUGGING
 +static pthread_mutex_t