[lxc-devel] [PATCH] Protect global variables in log module via mutex
Serge Hallyn
serge.hallyn at ubuntu.com
Mon Nov 11 21:04:11 UTC 2013
Quoting S.Çağlar Onur (caglar at 10ur.org):
> Log module contains multiple global variables so protect them introducing a new mutex and serialize accessing log functions.
> Also gather all locking related code into src/lxc/lxclock.c
>
> Signed-off-by: S.Çağlar Onur <caglar at 10ur.org>
Really the log stuff should be re-thought. What should happen right
now if two threads both call lxcapi_start() on containers with
lxc.logfile entries? Perhaps we need two sets of log info. One for
the program being used, and one for the running container. Anything
done after src/lxc/start.c:lxc_start() logs to the container log info -
that's anyhthing relating to container setup, container monitor stuff,
hooks, and the running of the container. Anything else is done to
the global log info - as that'll be shared by all threads.
Hopefully someone finds this interesting enough to write a patch :)
In the meantime - the infrastructure of this patch seems good, but
I don't think it really achieves protection of those variables.
log_fname and lxc_log_fd especially, because __lxc_log_set_file()
can close/free them concurrent with other __lxc_log_set_file()
runs and concurrent with lxc_log_get_file().
What do you think would be the best way to achieve that?
> ---
> src/lxc/log.c | 16 ++++++++++
> src/lxc/log.h | 4 +++
> src/lxc/lxclock.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++---------
> src/lxc/lxclock.h | 5 ++++
> src/lxc/utils.c | 57 +-----------------------------------
> 5 files changed, 100 insertions(+), 69 deletions(-)
>
> diff --git a/src/lxc/log.c b/src/lxc/log.c
> index d6ce361..8a5c511 100644
> --- a/src/lxc/log.c
> +++ b/src/lxc/log.c
> @@ -265,11 +265,15 @@ static int __lxc_log_set_file(const char *fname, int create_dirs)
> return -1;
> }
>
> + log_lock();
> lxc_log_fd = log_open(fname);
> + log_unlock();
> if (lxc_log_fd == -1)
> return -1;
>
> + log_lock();
> log_fname = strdup(fname);
> + log_unlock();
> return 0;
> }
>
> @@ -306,15 +310,19 @@ extern int lxc_log_init(const char *name, const char *file,
> return -1;
> }
>
> + log_lock();
> lxc_loglevel_specified = 1;
> lxc_priority = lxc_log_priority_to_int(priority);
> + log_unlock();
> }
>
> + log_lock();
> lxc_log_category_lxc.priority = lxc_priority;
> lxc_log_category_lxc.appender = &log_appender_logfile;
>
> if (!quiet)
> lxc_log_category_lxc.appender->next = &log_appender_stderr;
> + log_unlock();
>
> if (prefix)
> lxc_log_set_prefix(prefix);
> @@ -322,7 +330,9 @@ extern int lxc_log_init(const char *name, const char *file,
> if (file) {
> if (strcmp(file, "none") == 0)
> return 0;
> + log_lock();
> lxc_logfile_specified = 1;
> + log_unlock();
> ret = __lxc_log_set_file(file, 1);
> } else {
> ret = -1;
> @@ -368,8 +378,10 @@ extern int lxc_log_set_level(int level)
> ERROR("invalid log priority %d", level);
> return -1;
> }
> + log_lock();
> lxc_loglevel_specified = 1;
> lxc_log_category_lxc.priority = level;
> + log_unlock();
> return 0;
> }
>
> @@ -397,7 +409,9 @@ extern int lxc_log_set_file(const char *fname)
> {
> if (lxc_logfile_specified)
> return 0;
> + log_lock();
> lxc_logfile_specified = 1;
> + log_unlock();
> return __lxc_log_set_file(fname, 0);
> }
>
> @@ -408,8 +422,10 @@ extern const char *lxc_log_get_file(void)
>
> extern void lxc_log_set_prefix(const char *prefix)
> {
> + log_lock();
> strncpy(log_prefix, prefix, sizeof(log_prefix));
> log_prefix[sizeof(log_prefix) - 1] = 0;
> + log_unlock();
> }
>
> extern const char *lxc_log_get_prefix(void)
> diff --git a/src/lxc/log.h b/src/lxc/log.h
> index d3c40fb..59e8dd6 100644
> --- a/src/lxc/log.h
> +++ b/src/lxc/log.h
> @@ -30,6 +30,8 @@
> #include <string.h>
> #include <stdbool.h>
>
> +#include "lxclock.h"
> +
> #ifndef O_CLOEXEC
> #define O_CLOEXEC 02000000
> #endif
> @@ -183,6 +185,7 @@ static inline void LXC_##PRIORITY(struct lxc_log_locinfo *, \
> static inline void LXC_##PRIORITY(struct lxc_log_locinfo* locinfo, \
> const char* format, ...) \
> { \
> + log_lock(); \
> if (lxc_log_priority_is_enabled(acategory, \
> LXC_LOG_PRIORITY_##PRIORITY)) { \
> struct lxc_log_event evt = { \
> @@ -200,6 +203,7 @@ static inline void LXC_##PRIORITY(struct lxc_log_locinfo* locinfo, \
> __lxc_log(acategory, &evt); \
> va_end(va_ref); \
> } \
> + log_unlock(); \
> }
>
> /*
> diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
> index 3857ff0..a255acb 100644
> --- a/src/lxc/lxclock.c
> +++ b/src/lxc/lxclock.c
> @@ -31,6 +31,10 @@
> #include <lxc/log.h>
> #include <lxc/lxccontainer.h>
>
> +#ifdef MUTEX_DEBUGGING
> +#include <execinfo.h>
> +#endif
> +
> #define OFLAG (O_CREAT | O_RDWR)
> #define SEMMODE 0660
> #define SEMVALUE 1
> @@ -40,10 +44,57 @@ lxc_log_define(lxc_lock, lxc);
>
> #ifdef MUTEX_DEBUGGING
> pthread_mutex_t thread_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
> +pthread_mutex_t log_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
> +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 is not thread safe
> + fprintf(stderr, "\tObtained %zd stack frames.\n", size);
> +
> + for (i = 0; i < size; i++)
> + fprintf(stderr, "\t\t%s\n", strings[i]);
> +
> + free (strings);
> +}
> #else
> pthread_mutex_t thread_mutex = PTHREAD_MUTEX_INITIALIZER;
> +pthread_mutex_t log_mutex = PTHREAD_MUTEX_INITIALIZER;
> +pthread_mutex_t static_mutex = PTHREAD_MUTEX_INITIALIZER;
> +
> +inline void dump_stacktrace(void) {;}
> #endif
>
> +void lock_mutex(pthread_mutex_t *l)
> +{
> + int ret;
> +
> + if ((ret = pthread_mutex_lock(l)) != 0) {
> + fprintf(stderr, "pthread_mutex_lock returned:%d %s", ret, strerror(ret));
> + dump_stacktrace();
> + exit(1);
> + }
> +}
> +
> +void unlock_mutex(pthread_mutex_t *l)
> +{
> + int ret;
> +
> + if ((ret = pthread_mutex_unlock(l)) != 0) {
> + fprintf(stderr, "pthread_mutex_lock returned:%d %s", ret, strerror(ret));
> + dump_stacktrace();
> + exit(1);
> + }
> +}
> +
> static char *lxclock_name(const char *p, const char *n)
> {
> int ret;
> @@ -267,24 +318,34 @@ void lxc_putlock(struct lxc_lock *l)
>
> void process_lock(void)
> {
> - int ret;
> -
> - if ((ret = pthread_mutex_lock(&thread_mutex)) != 0) {
> - ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
> - dump_stacktrace();
> - exit(1);
> - }
> + lock_mutex(&thread_mutex);
> }
>
> void process_unlock(void)
> {
> - int ret;
> + unlock_mutex(&thread_mutex);
> +}
>
> - if ((ret = pthread_mutex_unlock(&thread_mutex)) != 0) {
> - ERROR("pthread_mutex_unlock returned:%d %s", ret, strerror(ret));
> - dump_stacktrace();
> - exit(1);
> - }
> +/* Protects global log variables and serializes the writes */
> +void log_lock(void)
> +{
> + lock_mutex(&log_mutex);
> +}
> +
> +void log_unlock(void)
> +{
> + unlock_mutex(&log_mutex);
> +}
> +
> +/* Protects static const values inside the lxc_global_config_value funtion */
> +void static_lock(void)
> +{
> + lock_mutex(&static_mutex);
> +}
> +
> +void static_unlock(void)
> +{
> + unlock_mutex(&static_mutex);
> }
>
> int container_mem_lock(struct lxc_container *c)
> diff --git a/src/lxc/lxclock.h b/src/lxc/lxclock.h
> index dcdf79d..a9d7bd4 100644
> --- a/src/lxc/lxclock.h
> +++ b/src/lxc/lxclock.h
> @@ -87,6 +87,11 @@ extern void lxc_putlock(struct lxc_lock *l);
>
> extern void process_lock(void);
> extern void process_unlock(void);
> +extern void log_lock(void);
> +extern void log_unlock(void);
> +extern void static_lock(void);
> +extern void static_unlock(void);
> +
> struct lxc_container;
> extern int container_mem_lock(struct lxc_container *c);
> extern void container_mem_unlock(struct lxc_container *c);
> diff --git a/src/lxc/utils.c b/src/lxc/utils.c
> index 4bc2c35..3fab9ae 100644
> --- a/src/lxc/utils.c
> +++ b/src/lxc/utils.c
> @@ -39,11 +39,6 @@
> #include <sys/types.h>
> #include <sys/wait.h>
> #include <assert.h>
> -#include <pthread.h>
> -
> -#ifdef MUTEX_DEBUGGING
> -#include <execinfo.h>
> -#endif
>
> #ifndef HAVE_GETLINE
> #ifdef HAVE_FGETLN
> @@ -59,57 +54,6 @@
>
> 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 is not thread safe
> - fprintf(stderr, "\tObtained %zd stack frames.\n", size);
> -
> - for (i = 0; i < size; i++)
> - fprintf(stderr, "\t\t%s\n", strings[i]);
> -
> - free (strings);
> -}
> -#else
> -static pthread_mutex_t static_mutex = PTHREAD_MUTEX_INITIALIZER;
> -
> -inline void dump_stacktrace(void) {;}
> -#endif
> -
> -/* Protects static const values inside the lxc_global_config_value funtion */
> -static void static_lock(void)
> -{
> - int ret;
> -
> - if ((ret = pthread_mutex_lock(&static_mutex)) != 0) {
> - ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
> - dump_stacktrace();
> - exit(1);
> - }
> -}
> -
> -static void static_unlock(void)
> -{
> - int ret;
> -
> - if ((ret = pthread_mutex_unlock(&static_mutex)) != 0) {
> - ERROR("pthread_mutex_unlock returned:%d %s", ret, strerror(ret));
> - dump_stacktrace();
> - exit(1);
> - }
> -}
> -
> static int _recursive_rmdir_onedev(char *dirname, dev_t pdev)
> {
> struct dirent dirent, *direntp;
> @@ -392,6 +336,7 @@ out:
> if (fin)
> fclose(fin);
> process_unlock();
> +
> static_lock();
> value = values[i];
> static_unlock();
> --
> 1.8.3.2
>
>
> ------------------------------------------------------------------------------
> November Webinars for C, C++, Fortran Developers
> Accelerate application performance with scalable programming models. Explore
> techniques for threading, error checking, porting, and tuning. Get the most
> from the latest Intel processors and coprocessors. See abstracts and register
> http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk
> _______________________________________________
> Lxc-devel mailing list
> Lxc-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lxc-devel
More information about the lxc-devel
mailing list