From f44d55f6c23ef676d9155ddcff2a16b2e00a3675 Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Fri, 9 Dec 2022 17:30:31 +0100 Subject: [PATCH] handle read from timerfd correctly When reading the timerfd gives an error, we should return right away because the timeout did not happen. If we change the timerfd timeout before reading it, we can get -EAGAIN. Don't log an error in that case but wait for the new timeout. --- spa/plugins/alsa/alsa-pcm.c | 15 +++++++++++++-- spa/plugins/alsa/alsa-seq.c | 10 ++++++++-- spa/plugins/audiotestsrc/audiotestsrc.c | 16 ++++++++++++---- spa/plugins/avb/avb-pcm.c | 12 +++++++----- spa/plugins/bluez5/media-sink.c | 19 +++++++++++++++---- spa/plugins/bluez5/media-source.c | 18 ++++++++++++++---- spa/plugins/bluez5/sco-sink.c | 19 ++++++++++++++----- spa/plugins/bluez5/sco-source.c | 12 ++++++++++-- spa/plugins/support/node-driver.c | 13 ++++++++----- spa/plugins/support/null-audio-sink.c | 13 ++++++++----- spa/plugins/test/fakesink.c | 16 +++++++++++----- spa/plugins/test/fakesrc.c | 14 ++++++++++---- spa/plugins/videotestsrc/videotestsrc.c | 16 ++++++++++++---- spa/plugins/vulkan/vulkan-compute-source.c | 15 +++++++++++---- 14 files changed, 153 insertions(+), 55 deletions(-) diff --git a/spa/plugins/alsa/alsa-pcm.c b/spa/plugins/alsa/alsa-pcm.c index 5e0a60b37..87d1656aa 100644 --- a/spa/plugins/alsa/alsa-pcm.c +++ b/spa/plugins/alsa/alsa-pcm.c @@ -2428,9 +2428,20 @@ static void alsa_on_timeout_event(struct spa_source *source) struct state *state = source->data; snd_pcm_uframes_t delay, target; uint64_t expire, current_time; + int res; - if (SPA_UNLIKELY(state->started && spa_system_timerfd_read(state->data_system, state->timerfd, &expire) < 0)) - spa_log_warn(state->log, "%p: error reading timerfd: %m", state); + if (SPA_LIKELY(state->started)) { + if (SPA_UNLIKELY((res = spa_system_timerfd_read(state->data_system, + state->timerfd, &expire)) < 0)) { + /* we can get here when the timer is changed since the last + * timerfd wakeup, for example by do_reassign_follower() executed + * in the same epoll wakeup cycle */ + if (res != -EAGAIN) + spa_log_warn(state->log, "%p: error reading timerfd: %s", + state, spa_strerror(res)); + return; + } + } check_position_config(state); diff --git a/spa/plugins/alsa/alsa-seq.c b/spa/plugins/alsa/alsa-seq.c index 407b88f84..9cec44d2d 100644 --- a/spa/plugins/alsa/alsa-seq.c +++ b/spa/plugins/alsa/alsa-seq.c @@ -800,8 +800,14 @@ static void alsa_on_timeout_event(struct spa_source *source) uint64_t expire; int res; - if (state->started && spa_system_timerfd_read(state->data_system, state->timerfd, &expire) < 0) - spa_log_warn(state->log, "error reading timerfd: %m"); + if (state->started) { + if ((res = spa_system_timerfd_read(state->data_system, state->timerfd, &expire)) < 0) { + if (res != -EAGAIN) + spa_log_warn(state->log, "%p: error reading timerfd: %s", + state, spa_strerror(res)); + return; + } + } state->current_time = state->next_time; diff --git a/spa/plugins/audiotestsrc/audiotestsrc.c b/spa/plugins/audiotestsrc/audiotestsrc.c index f009b9a29..c0c9c7a68 100644 --- a/spa/plugins/audiotestsrc/audiotestsrc.c +++ b/spa/plugins/audiotestsrc/audiotestsrc.c @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -348,14 +349,20 @@ static void set_timer(struct impl *this, bool enabled) } } -static void read_timer(struct impl *this) +static int read_timer(struct impl *this) { uint64_t expirations; + int res = 0; if (this->async || this->props.live) { - if (spa_system_timerfd_read(this->data_system, this->timer_source.fd, &expirations) < 0) - perror("read timerfd"); + if ((res = spa_system_timerfd_read(this->data_system, + this->timer_source.fd, &expirations)) < 0) { + if (res != -EAGAIN) + spa_log_error(this->log, NAME " %p: timerfd error: %s", + this, spa_strerror(res)); + } } + return 0; } static int make_buffer(struct impl *this) @@ -369,7 +376,8 @@ static int make_buffer(struct impl *this) uint32_t filled, avail; uint32_t index, offset, l0, l1; - read_timer(this); + if (read_timer(this) < 0) + return 0; if (spa_list_is_empty(&port->empty)) { set_timer(this, false); diff --git a/spa/plugins/avb/avb-pcm.c b/spa/plugins/avb/avb-pcm.c index 8fe9503a3..484adc660 100644 --- a/spa/plugins/avb/avb-pcm.c +++ b/spa/plugins/avb/avb-pcm.c @@ -36,6 +36,7 @@ #include #include +#include #include #include #include @@ -1048,14 +1049,15 @@ static void avb_on_timeout_event(struct spa_source *source) struct state *state = source->data; uint64_t expirations, current_time, duration; uint32_t rate; + int res; spa_log_trace(state->log, "timeout"); - if (spa_system_timerfd_read(state->data_system, - state->timer_source.fd, &expirations) < 0) { - if (errno == EAGAIN) - return; - spa_log_error(state->log, "read timerfd: %m"); + if ((res = spa_system_timerfd_read(state->data_system, + state->timer_source.fd, &expirations)) < 0) { + if (res != -EAGAIN) + spa_log_error(state->log, "read timerfd: %s", spa_strerror(res)); + return; } current_time = state->next_time; diff --git a/spa/plugins/bluez5/media-sink.c b/spa/plugins/bluez5/media-sink.c index a5d72fcfc..ee6cef4ce 100644 --- a/spa/plugins/bluez5/media-sink.c +++ b/spa/plugins/bluez5/media-sink.c @@ -839,11 +839,15 @@ static void media_on_flush_timeout(struct spa_source *source) { struct impl *this = source->data; uint64_t exp; + int res; spa_log_trace(this->log, "%p: flush on timeout", this); - if (spa_system_timerfd_read(this->data_system, this->flush_timerfd, &exp) < 0) - spa_log_warn(this->log, "error reading timerfd: %s", strerror(errno)); + if ((res = spa_system_timerfd_read(this->data_system, this->flush_timerfd, &exp)) < 0) { + if (res != -EAGAIN) + spa_log_warn(this->log, "error reading timerfd: %s", spa_strerror(res)); + return; + } if (this->transport == NULL) { enable_flush_timer(this, false); @@ -864,12 +868,19 @@ static void media_on_timeout(struct spa_source *source) uint32_t rate; struct spa_io_buffers *io = port->io; uint64_t prev_time, now_time; + int res; if (this->transport == NULL) return; - if (this->started && spa_system_timerfd_read(this->data_system, this->timerfd, &exp) < 0) - spa_log_warn(this->log, "error reading timerfd: %s", strerror(errno)); + if (this->started) { + if ((res = spa_system_timerfd_read(this->data_system, this->timerfd, &exp)) < 0) { + if (res != -EAGAIN) + spa_log_warn(this->log, "error reading timerfd: %s", + spa_strerror(res)); + return; + } + } prev_time = this->current_time; now_time = this->current_time = this->next_time; diff --git a/spa/plugins/bluez5/media-source.c b/spa/plugins/bluez5/media-source.c index 58ff14a52..d260335ce 100644 --- a/spa/plugins/bluez5/media-source.c +++ b/spa/plugins/bluez5/media-source.c @@ -538,9 +538,13 @@ static void media_on_duplex_timeout(struct spa_source *source) { struct impl *this = source->data; uint64_t exp; + int res; - if (spa_system_timerfd_read(this->data_system, this->duplex_timerfd, &exp) < 0) - spa_log_warn(this->log, "error reading timerfd: %s", strerror(errno)); + if ((res = spa_system_timerfd_read(this->data_system, this->duplex_timerfd, &exp)) < 0) { + if (res != -EAGAIN) + spa_log_warn(this->log, "error reading timerfd: %s", spa_strerror(res)); + return; + } set_duplex_timeout(this, this->duplex_timeout); @@ -577,12 +581,18 @@ static void media_on_timeout(struct spa_source *source) uint64_t exp, duration; uint32_t rate; uint64_t prev_time, now_time; + int res; if (this->transport == NULL) return; - if (this->started && spa_system_timerfd_read(this->data_system, this->timerfd, &exp) < 0) - spa_log_warn(this->log, "error reading timerfd: %s", strerror(errno)); + if (this->started) { + if ((res = spa_system_timerfd_read(this->data_system, this->timerfd, &exp)) < 0) { + if (res != -EAGAIN) + spa_log_warn(this->log, "error reading timerfd: %s", spa_strerror(res)); + return; + } + } prev_time = this->current_time; now_time = this->current_time = this->next_time; diff --git a/spa/plugins/bluez5/sco-sink.c b/spa/plugins/bluez5/sco-sink.c index e6a7288f9..7025c78a9 100644 --- a/spa/plugins/bluez5/sco-sink.c +++ b/spa/plugins/bluez5/sco-sink.c @@ -567,16 +567,19 @@ stop: enable_flush_timer(this, false); } - static void sco_on_flush_timeout(struct spa_source *source) { struct impl *this = source->data; uint64_t exp; + int res; spa_log_trace(this->log, "%p: flush on timeout", this); - if (spa_system_timerfd_read(this->data_system, this->flush_timerfd, &exp) < 0) - spa_log_warn(this->log, "error reading timerfd: %s", strerror(errno)); + if ((res = spa_system_timerfd_read(this->data_system, this->flush_timerfd, &exp)) < 0) { + if (res != -EAGAIN) + spa_log_warn(this->log, "error reading timerfd: %s", spa_strerror(res)); + return; + } if (this->transport == NULL) { enable_flush_timer(this, false); @@ -597,12 +600,18 @@ static void sco_on_timeout(struct spa_source *source) uint32_t rate; struct spa_io_buffers *io = port->io; uint64_t prev_time, now_time; + int res; if (this->transport == NULL) return; - if (this->started && spa_system_timerfd_read(this->data_system, this->timerfd, &exp) < 0) - spa_log_warn(this->log, "error reading timerfd: %s", strerror(errno)); + if (this->started) { + if ((res = spa_system_timerfd_read(this->data_system, this->timerfd, &exp)) < 0) { + if (res != -EAGAIN) + spa_log_warn(this->log, "error reading timerfd: %s", spa_strerror(res)); + return; + } + } prev_time = this->current_time; now_time = this->current_time = this->next_time; diff --git a/spa/plugins/bluez5/sco-source.c b/spa/plugins/bluez5/sco-source.c index 17855065f..b4c40983a 100644 --- a/spa/plugins/bluez5/sco-source.c +++ b/spa/plugins/bluez5/sco-source.c @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -600,12 +601,19 @@ static void sco_on_timeout(struct spa_source *source) uint64_t exp, duration; uint32_t rate; uint64_t prev_time, now_time; + int res; if (this->transport == NULL) return; - if (this->started && spa_system_timerfd_read(this->data_system, this->timerfd, &exp) < 0) - spa_log_warn(this->log, "error reading timerfd: %s", strerror(errno)); + if (this->started) { + if ((res = spa_system_timerfd_read(this->data_system, this->timerfd, &exp)) < 0) { + if (res != -EAGAIN) + spa_log_warn(this->log, "error reading timerfd: %s", + spa_strerror(res)); + return; + } + } prev_time = this->current_time; now_time = this->current_time = this->next_time; diff --git a/spa/plugins/support/node-driver.c b/spa/plugins/support/node-driver.c index 7fb6027ba..9701a478c 100644 --- a/spa/plugins/support/node-driver.c +++ b/spa/plugins/support/node-driver.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -180,14 +181,16 @@ static void on_timeout(struct spa_source *source) struct impl *this = source->data; uint64_t expirations, nsec, duration; uint32_t rate; + int res; spa_log_trace(this->log, "timeout"); - if (spa_system_timerfd_read(this->data_system, - this->timer_source.fd, &expirations) < 0) { - if (errno == EAGAIN) - return; - perror("read timerfd"); + if ((res = spa_system_timerfd_read(this->data_system, + this->timer_source.fd, &expirations)) < 0) { + if (res != EAGAIN) + spa_log_error(this->log, NAME " %p: timerfd error: %s", + this, spa_strerror(res)); + return; } nsec = this->next_time; diff --git a/spa/plugins/support/null-audio-sink.c b/spa/plugins/support/null-audio-sink.c index e42c3c1c8..c39c15afb 100644 --- a/spa/plugins/support/null-audio-sink.c +++ b/spa/plugins/support/null-audio-sink.c @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include @@ -282,14 +283,16 @@ static void on_timeout(struct spa_source *source) struct impl *this = source->data; uint64_t expirations, nsec, duration = 10; uint32_t rate; + int res; spa_log_trace(this->log, "timeout"); - if (spa_system_timerfd_read(this->data_system, - this->timer_source.fd, &expirations) < 0) { - if (errno == EAGAIN) - return; - perror("read timerfd"); + if ((res = spa_system_timerfd_read(this->data_system, + this->timer_source.fd, &expirations)) < 0) { + if (res != EAGAIN) + spa_log_error(this->log, NAME " %p: timerfd error: %s", + this, spa_strerror(res)); + return; } nsec = this->next_time; diff --git a/spa/plugins/test/fakesink.c b/spa/plugins/test/fakesink.c index 104f27eb9..5df4bbb0f 100644 --- a/spa/plugins/test/fakesink.c +++ b/spa/plugins/test/fakesink.c @@ -215,15 +215,20 @@ static void set_timer(struct impl *this, bool enabled) } } -static inline void read_timer(struct impl *this) +static inline int read_timer(struct impl *this) { uint64_t expirations; + int res = 0; if (this->callbacks.funcs || this->props.live) { - if (spa_system_timerfd_read(this->data_system, - this->timer_source.fd, &expirations) < 0) - perror("read timerfd"); + if ((res = spa_system_timerfd_read(this->data_system, + this->timer_source.fd, &expirations)) < 0) { + if (res != -EAGAIN) + spa_log_error(this->log, NAME " %p: timerfd error: %s", + this, spa_strerror(res)); + } } + return res; } static void render_buffer(struct impl *this, struct buffer *b) @@ -237,7 +242,8 @@ static int consume_buffer(struct impl *this) struct spa_io_buffers *io = port->io; int n_bytes; - read_timer(this); + if (read_timer(this) < 0) + return 0; if (spa_list_is_empty(&port->ready)) { io->status = SPA_STATUS_NEED_DATA; diff --git a/spa/plugins/test/fakesrc.c b/spa/plugins/test/fakesrc.c index 6dde68455..5b3d409c1 100644 --- a/spa/plugins/test/fakesrc.c +++ b/spa/plugins/test/fakesrc.c @@ -233,12 +233,17 @@ static void set_timer(struct impl *this, bool enabled) static inline void read_timer(struct impl *this) { uint64_t expirations; + int res = 0; if (this->callbacks.funcs || this->props.live) { - if (spa_system_timerfd_read(this->data_system, - this->timer_source.fd, &expirations) < 0) - perror("read timerfd"); + if ((res = spa_system_timerfd_read(this->data_system, + this->timer_source.fd, &expirations)) < 0) { + if (res != -EAGAIN) + spa_log_error(this->log, NAME " %p: timerfd error: %s", + this, spa_strerror(res)); + } } + return res; } static int make_buffer(struct impl *this) @@ -248,7 +253,8 @@ static int make_buffer(struct impl *this) struct spa_io_buffers *io = port->io; int n_bytes; - read_timer(this); + if (read_timer(this) < 0) + return 0; if (spa_list_is_empty(&port->empty)) { set_timer(this, false); diff --git a/spa/plugins/videotestsrc/videotestsrc.c b/spa/plugins/videotestsrc/videotestsrc.c index f2f42c060..af3bc9688 100644 --- a/spa/plugins/videotestsrc/videotestsrc.c +++ b/spa/plugins/videotestsrc/videotestsrc.c @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -280,14 +281,20 @@ static void set_timer(struct impl *this, bool enabled) } } -static void read_timer(struct impl *this) +static int read_timer(struct impl *this) { uint64_t expirations; + int res = 0; if (this->async || this->props.live) { - if (spa_system_timerfd_read(this->data_system, this->timer_source.fd, &expirations) < 0) - perror("read timerfd"); + if ((res = spa_system_timerfd_read(this->data_system, + this->timer_source.fd, &expirations)) < 0) { + if (res != -EAGAIN) + spa_log_error(this->log, NAME " %p: timerfd error: %s", + this, spa_strerror(res)); + } } + return res; } static int make_buffer(struct impl *this) @@ -297,7 +304,8 @@ static int make_buffer(struct impl *this) struct spa_io_buffers *io = port->io; uint32_t n_bytes; - read_timer(this); + if (read_timer(this) < 0) + return 0; if (spa_list_is_empty(&port->empty)) { set_timer(this, false); diff --git a/spa/plugins/vulkan/vulkan-compute-source.c b/spa/plugins/vulkan/vulkan-compute-source.c index 8cce5489f..63a37e2ec 100644 --- a/spa/plugins/vulkan/vulkan-compute-source.c +++ b/spa/plugins/vulkan/vulkan-compute-source.c @@ -267,14 +267,20 @@ static void set_timer(struct impl *this, bool enabled) } } -static void read_timer(struct impl *this) +static int read_timer(struct impl *this) { uint64_t expirations; + int res = 0; if (this->async || this->props.live) { - if (spa_system_timerfd_read(this->data_system, this->timer_source.fd, &expirations) < 0) - perror("read timerfd"); + if ((res = spa_system_timerfd_read(this->data_system, + this->timer_source.fd, &expirations)) < 0) { + if (res != -EAGAIN) + spa_log_error(this->log, NAME " %p: timerfd error: %s", + this, spa_strerror(res)); + } } + return res; } static int make_buffer(struct impl *this) @@ -284,7 +290,8 @@ static int make_buffer(struct impl *this) uint32_t n_bytes; int res; - read_timer(this); + if (read_timer(this) < 0) + return 0; if ((res = spa_vulkan_ready(&this->state)) < 0) { res = SPA_STATUS_OK;