diff options
authorwm4 <wm4@nowhere>2020-01-30 14:00:41 +0100
committerwm4 <wm4@nowhere>2020-01-30 14:13:35 +0100
commit355bb5b1e68ad1281d21d891b25f3bc917c307ff (patch)
parent2fd34889fe7732eafc60e4acaf20c4fced9a4a81 (diff)
msg: fix some locking issues
The wakeup_log_file callback was still assuming that mp_msg_lock was used to control the log file thread, but this changed while I was writing this code, and forgot to update it. (It doesn't change any state, which is untypical for condition variable usage. The state that is changed is protected by another lock instead. But log_file_lock still needs to be acquired to ensure the signal isn't sent while the thread is right before the pthread_cond_wait() call, when the lock is held, but the signal would still be lost.) Because the buffer's wakeup callback now acquires the lock, the wakeup callback must be called outside of the buffer lock, to keep the lock order (log_file_lock > mp_log_buffer.lock). Fortunately, the wakeup callback is immutable, or we would have needed another dumb leaf lock. mp_msg_has_log_file() made a similar outdated assumption. But now access to the log_file field is much trickier; just define that it's only to be called from the thread that manages the msg state. (The calling code could also just check whether the log-file option changed instead, but currently that would be slightly more messy.)
1 files changed, 8 insertions, 7 deletions
diff --git a/common/msg.c b/common/msg.c
index b78beb3ad5..336a6d67cc 100644
--- a/common/msg.c
+++ b/common/msg.c
@@ -318,6 +318,7 @@ static void write_msg_to_buffers(struct mp_log *log, int lev, char *text)
struct mp_log_root *root = log->root;
for (int n = 0; n < root->num_buffers; n++) {
struct mp_log_buffer *buffer = root->buffers[n];
+ bool wakeup = false;
int buffer_level = buffer->level;
if (buffer_level == MP_LOG_BUFFER_MSGL_TERM)
@@ -358,9 +359,11 @@ static void write_msg_to_buffers(struct mp_log *log, int lev, char *text)
buffer->entries[pos] = entry;
buffer->num_entries += 1;
if (buffer->wakeup_cb && !buffer->silent)
- buffer->wakeup_cb(buffer->wakeup_cb_ctx);
+ wakeup = true;
+ if (wakeup)
+ buffer->wakeup_cb(buffer->wakeup_cb_ctx);
@@ -533,8 +536,9 @@ static void wakeup_log_file(void *p)
struct mp_log_root *root = p;
- // This makes use of the implicit fact that the caller holds mp_msg_lock.
+ pthread_mutex_lock(&root->log_file_lock);
+ pthread_mutex_unlock(&root->log_file_lock);
// Only to be called from the main thread.
@@ -658,15 +662,12 @@ void mp_msg_force_stderr(struct mpv_global *global, bool force_stderr)
+// Only to be called from the main thread.
bool mp_msg_has_log_file(struct mpv_global *global)
struct mp_log_root *root = global->log->root;
- pthread_mutex_lock(&mp_msg_lock);
- bool res = !!root->log_file;
- pthread_mutex_unlock(&mp_msg_lock);
- return res;
+ return !!root->log_file;
void mp_msg_uninit(struct mpv_global *global)