diff options
author | wm4 <wm4@nowhere> | 2020-01-30 14:00:41 +0100 |
---|---|---|
committer | wm4 <wm4@nowhere> | 2020-01-30 14:13:35 +0100 |
commit | 355bb5b1e68ad1281d21d891b25f3bc917c307ff (patch) | |
tree | 1e856d924a5dcb56c8eda8eaf2408975a3588b62 /common/msg.c | |
parent | 2fd34889fe7732eafc60e4acaf20c4fced9a4a81 (diff) | |
download | mpv-355bb5b1e68ad1281d21d891b25f3bc917c307ff.tar.bz2 mpv-355bb5b1e68ad1281d21d891b25f3bc917c307ff.tar.xz |
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.)
Diffstat (limited to 'common/msg.c')
-rw-r--r-- | common/msg.c | 15 |
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; pthread_mutex_lock(&buffer->lock); 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; } pthread_mutex_unlock(&buffer->lock); + 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_cond_broadcast(&root->log_file_wakeup); + 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) pthread_mutex_unlock(&mp_msg_lock); } +// 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) |