[lxc-devel] [PATCH] Protect global variables in log module via mutex
S.Çağlar Onur
caglar at 10ur.org
Tue Nov 12 17:03:10 UTC 2013
Hi Serge,
On Mon, Nov 11, 2013 at 4:04 PM, Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> 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.
Agreed.
> 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?
Hmmm just an idea without giving lots of thought but considering the
objective above what about storing those variables in container
struct, adding a new method to API like c->log(c, MESSAGE, LEVEL) (or
some helpers like APIERROR(c, MESSAGE), APIWARNING etc) and re-using
parts of the log module there.
Come to think of it, do we really need to have a global/shared logging
at all? What do you think making the whole logging thing to container
specific?
>> ---
>> 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
--
S.Çağlar Onur <caglar at 10ur.org>
More information about the lxc-devel
mailing list