vlog: Make thread-safe.

Signed-off-by: Ben Pfaff <blp@nicira.com>

lib/vlog.c

@@ -33,6 +33,7 @@
 #include "dirs.h"
 #include "dynamic-string.h"
 #include "ofpbuf.h"
+#include "ovs-thread.h"
 #include "sat-math.h"
 #include "svec.h"
 #include "timeval.h"
@@ -84,7 +85,7 @@ struct vlog_module *vlog_modules[] = {
 /* Information about each facility. */
 struct facility {
     const char *name;           /* Name. */
-    char *pattern;              /* Current pattern. */
+    char *pattern;              /* Current pattern (see 'pattern_rwlock'). */
     bool default_pattern;       /* Whether current pattern is the default. */
 };
 static struct facility facilities[VLF_N_FACILITIES] = {
@@ -93,18 +94,27 @@ static struct facility facilities[VLF_N_FACILITIES] = {
 #undef VLOG_FACILITY
 };
 
-/* VLF_FILE configuration. */
+/* Protects the 'pattern' in all "struct facility"s, so that a race between
+ * changing and reading the pattern does not cause an access to freed
+ * memory. */
+static pthread_rwlock_t pattern_rwlock = PTHREAD_RWLOCK_INITIALIZER;
+
+/* Sequence number for the message currently being composed. */
+DEFINE_PER_THREAD_DATA(unsigned int, msg_num, 0);
+
+/* VLF_FILE configuration.
+ *
+ * All of the following is protected by 'log_file_mutex', which nests inside
+ * pattern_rwlock. */
+static pthread_mutex_t log_file_mutex = PTHREAD_ADAPTIVE_MUTEX_INITIALIZER;
 static char *log_file_name;
 static int log_fd = -1;
 static struct async_append *log_writer;
 
-/* vlog initialized? */
-static bool vlog_inited;
-
 static void format_log_message(const struct vlog_module *, enum vlog_level,
-                               enum vlog_facility, unsigned int msg_num,
+                               enum vlog_facility,
                                const char *message, va_list, struct ds *)
-    PRINTF_FORMAT(5, 0);
+    PRINTF_FORMAT(4, 0);
 
 /* Searches the 'n_names' in 'names'.  Returns the index of a match for
  * 'target', or 'n_names' if no name matches. */
@@ -191,7 +201,7 @@ vlog_get_level(const struct vlog_module *module, enum vlog_facility facility)
     return module->levels[facility];
 }
 
-static void
+static void OVS_MUST_HOLD(log_file_mutex)
 update_min_level(struct vlog_module *module)
 {
     enum vlog_facility facility;
@@ -214,6 +224,7 @@ set_facility_level(enum vlog_facility facility, struct vlog_module *module,
     assert(facility >= 0 && facility < VLF_N_FACILITIES);
     assert(level < VLL_N_LEVELS);
 
+    xpthread_mutex_lock(&log_file_mutex);
     if (!module) {
         struct vlog_module **mp;
 
@@ -225,6 +236,7 @@ set_facility_level(enum vlog_facility facility, struct vlog_module *module,
         module->levels[facility] = level;
         update_min_level(module);
     }
+    xpthread_mutex_unlock(&log_file_mutex);
 }
 
 /* Sets the logging level for the given 'module' and 'facility' to 'level'.  A
@@ -248,12 +260,15 @@ static void
 do_set_pattern(enum vlog_facility facility, const char *pattern)
 {
     struct facility *f = &facilities[facility];
+
+    xpthread_rwlock_wrlock(&pattern_rwlock);
     if (!f->default_pattern) {
         free(f->pattern);
     } else {
         f->default_pattern = false;
     }
     f->pattern = xstrdup(pattern);
+    xpthread_rwlock_unlock(&pattern_rwlock);
 }
 
 /* Sets the pattern for the given 'facility' to 'pattern'. */
@@ -276,49 +291,67 @@ vlog_set_pattern(enum vlog_facility facility, const char *pattern)
 int
 vlog_set_log_file(const char *file_name)
 {
-    char *old_log_file_name;
+    char *new_log_file_name;
     struct vlog_module **mp;
-    int error;
+    struct stat old_stat;
+    struct stat new_stat;
+    int new_log_fd;
+    bool same_file;
 
-    /* Close old log file. */
+    /* Open new log file. */
-    if (log_fd >= 0) {
+    new_log_file_name = (file_name
-        VLOG_INFO("closing log file");
+                         ? xstrdup(file_name)
-
+                         : xasprintf("%s/%s.log", ovs_logdir(), program_name));
-        async_append_destroy(log_writer);
+    new_log_fd = open(new_log_file_name, O_WRONLY | O_CREAT | O_APPEND, 0666);
-        log_writer = NULL;
+    if (new_log_fd < 0) {
-
+        VLOG_WARN("failed to open %s for logging: %s",
-        close(log_fd);
+                  new_log_file_name, ovs_strerror(errno));
-        log_fd = -1;
+        free(new_log_file_name);
+        return errno;
     }
 
-    /* Update log file name and free old name.  The ordering is important
+    /* If the new log file is the same one we already have open, bail out. */
-     * because 'file_name' might be 'log_file_name' or some suffix of it. */
+    xpthread_mutex_lock(&log_file_mutex);
-    old_log_file_name = log_file_name;
+    same_file = (log_fd >= 0
-    log_file_name = (file_name
+                 && new_log_fd >= 0
-                     ? xstrdup(file_name)
+                 && !fstat(log_fd, &old_stat)
-                     : xasprintf("%s/%s.log", ovs_logdir(), program_name));
+                 && !fstat(new_log_fd, &new_stat)
-    free(old_log_file_name);
+                 && old_stat.st_dev == new_stat.st_dev
-    file_name = NULL;           /* Might have been freed. */
+                 && old_stat.st_ino == new_stat.st_ino);
+    xpthread_mutex_unlock(&log_file_mutex);
+    if (same_file) {
+        close(new_log_fd);
+        free(new_log_file_name);
+        return 0;
+    }
+
+    /* Log closing old log file (we can't log while holding log_file_mutex). */
+    if (log_fd >= 0) {
+        VLOG_INFO("closing log file");
+    }
+
+    /* Close old log file, if any, and install new one. */
+    xpthread_mutex_lock(&log_file_mutex);
+    if (log_fd >= 0) {
+        free(log_file_name);
+        close(log_fd);
+        async_append_destroy(log_writer);
+    }
+
+    log_file_name = xstrdup(new_log_file_name);
+    log_fd = new_log_fd;
+    log_writer = async_append_create(new_log_fd);
 
-    /* Open new log file and update min_levels[] to reflect whether we actually
-     * have a log_file. */
-    log_fd = open(log_file_name, O_WRONLY | O_CREAT | O_APPEND, 0666);
     for (mp = vlog_modules; mp < &vlog_modules[n_vlog_modules]; mp++) {
         update_min_level(*mp);
     }
+    xpthread_mutex_unlock(&log_file_mutex);
 
-    /* Log success or failure. */
+    /* Log opening new log file (we can't log while holding log_file_mutex). */
-    if (log_fd < 0) {
+    VLOG_INFO("opened log file %s", new_log_file_name);
-        VLOG_WARN("failed to open %s for logging: %s",
+    free(new_log_file_name);
-                  log_file_name, ovs_strerror(errno));
-        error = errno;
-    } else {
-        log_writer = async_append_create(log_fd);
-        VLOG_INFO("opened log file %s", log_file_name);
-        error = 0;
-    }
 
-    return error;
+    return 0;
 }
 
 /* Closes and then attempts to re-open the current log file.  (This is useful
@@ -327,25 +360,19 @@ vlog_set_log_file(const char *file_name)
 int
 vlog_reopen_log_file(void)
 {
-    struct stat old, new;
+    char *fn;
 
-    /* Skip re-opening if there's nothing to reopen. */
+    xpthread_mutex_lock(&log_file_mutex);
-    if (!log_file_name) {
+    fn = log_file_name ? xstrdup(log_file_name) : NULL;
+    xpthread_mutex_unlock(&log_file_mutex);
+
+    if (fn) {
+        int error = vlog_set_log_file(fn);
+        free(fn);
+        return error;
+    } else {
         return 0;
     }
-
-    /* Skip re-opening if it would be a no-op because the old and new files are
-     * the same.  (This avoids writing "closing log file" followed immediately
-     * by "opened log file".) */
-    if (log_fd >= 0
-        && !fstat(log_fd, &old)
-        && !stat(log_file_name, &new)
-        && old.st_dev == new.st_dev
-        && old.st_ino == new.st_ino) {
-        return 0;
-    }
-
-    return vlog_set_log_file(log_file_name);
 }
 
 /* Set debugging levels.  Returns null if successful, otherwise an error
@@ -538,19 +565,12 @@ vlog_disable_rate_limit(struct unixctl_conn *conn, int argc,
     set_rate_limits(conn, argc, argv, false);
 }
 
-/* Initializes the logging subsystem and registers its unixctl server
+static void
- * commands. */
+vlog_init__(void)
-void
-vlog_init(void)
 {
     static char *program_name_copy;
     time_t now;
 
-    if (vlog_inited) {
-        return;
-    }
-    vlog_inited = true;
-
     /* openlog() is allowed to keep the pointer passed in, without making a
      * copy.  The daemonize code sometimes frees and replaces 'program_name',
      * so make a private copy just for openlog().  (We keep a pointer to the
@@ -578,6 +598,15 @@ vlog_init(void)
                              vlog_unixctl_reopen, NULL);
 }
 
+/* Initializes the logging subsystem and registers its unixctl server
+ * commands. */
+void
+vlog_init(void)
+{
+    static pthread_once_t once = PTHREAD_ONCE_INIT;
+    pthread_once(&once, vlog_init__);
+}
+
 /* Print the current logging level for each module. */
 char *
 vlog_get_levels(void)
@@ -643,7 +672,7 @@ fetch_braces(const char *p, const char *def, char *out, size_t out_size)
 
 static void
 format_log_message(const struct vlog_module *module, enum vlog_level level,
-                   enum vlog_facility facility, unsigned int msg_num,
+                   enum vlog_facility facility,
                    const char *message, va_list args_, struct ds *s)
 {
     char tmp[128];
@@ -705,7 +734,7 @@ format_log_message(const struct vlog_module *module, enum vlog_level level,
             }
             break;
         case 'N':
-            ds_put_format(s, "%u", msg_num);
+            ds_put_format(s, "%u", *msg_num_get_unsafe());
             break;
         case 'n':
             ds_put_char(s, '\n');
@@ -760,18 +789,17 @@ vlog_valist(const struct vlog_module *module, enum vlog_level level,
     bool log_to_file = module->levels[VLF_FILE] >= level && log_fd >= 0;
     if (log_to_console || log_to_syslog || log_to_file) {
         int save_errno = errno;
-        static unsigned int msg_num;
         struct ds s;
 
         vlog_init();
 
         ds_init(&s);
         ds_reserve(&s, 1024);
-        msg_num++;
+        ++*msg_num_get();
 
+        xpthread_rwlock_rdlock(&pattern_rwlock);
         if (log_to_console) {
-            format_log_message(module, level, VLF_CONSOLE, msg_num,
+            format_log_message(module, level, VLF_CONSOLE, message, args, &s);
-                               message, args, &s);
             ds_put_char(&s, '\n');
             fputs(ds_cstr(&s), stderr);
         }
@@ -781,8 +809,7 @@ vlog_valist(const struct vlog_module *module, enum vlog_level level,
             char *save_ptr = NULL;
             char *line;
 
-            format_log_message(module, level, VLF_SYSLOG, msg_num,
+            format_log_message(module, level, VLF_SYSLOG, message, args, &s);
-                               message, args, &s);
             for (line = strtok_r(s.string, "\n", &save_ptr); line;
                  line = strtok_r(NULL, "\n", &save_ptr)) {
                 syslog(syslog_level, "%s", line);
@@ -790,14 +817,19 @@ vlog_valist(const struct vlog_module *module, enum vlog_level level,
         }
 
         if (log_to_file) {
-            format_log_message(module, level, VLF_FILE, msg_num,
+            format_log_message(module, level, VLF_FILE, message, args, &s);
-                               message, args, &s);
             ds_put_char(&s, '\n');
-            async_append_write(log_writer, s.string, s.length);
+
-            if (level == VLL_EMER) {
+            xpthread_mutex_lock(&log_file_mutex);
-                async_append_flush(log_writer);
+            if (log_fd >= 0) {
+                async_append_write(log_writer, s.string, s.length);
+                if (level == VLL_EMER) {
+                    async_append_flush(log_writer);
+                }
             }
+            xpthread_mutex_unlock(&log_file_mutex);
         }
+        xpthread_rwlock_unlock(&pattern_rwlock);
 
         ds_destroy(&s);
         errno = save_errno;

lib/vlog.h

@@ -17,6 +17,15 @@
 #ifndef VLOG_H
 #define VLOG_H 1
 
+/* Logging.
+ *
+ *
+ * Thread-safety
+ * =============
+ *
+ * Fully thread safe.
+ */
+
 #include <limits.h>
 #include <stdarg.h>
 #include <stdbool.h>