[PATCH weston 1/2] clients: consolidate timer code part 1
Derek Foreman
derekf at osg.samsung.com
Fri Mar 9 17:32:47 UTC 2018
On 2018-03-09 05:44 AM, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
> There are multiple copies for the timerfd handling code, and I need a
> timer in one more app. Consolidate all the timerfd code into window.c to
> reduce the duplication. Many of the copies were also flawed against the
> race mentioned in toytimer_fire().
>
> This patch handles clickdot and window.c's tooltip timer and cursor
> timer.
>
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
Just out of curiosity, how'd you decide to break it down this way?
Both patches look good to me. I hate that reset/fire race and wasted a
bit of time understanding it the first time I hit it. I'm really happy
to see it handled consistently in one place.
Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
For both.
I think they look safe to land now, as it's mostly harmless refactor,
and the rest is actual bug fix, as abort()ing on the read fail was wrong.
I'll let you make the call on landing it. :)
Thanks,
Derek
> ---
> clients/clickdot.c | 36 +++----------
> clients/window.c | 150 +++++++++++++++++++++++++++++++++++------------------
> clients/window.h | 27 ++++++++++
> 3 files changed, 135 insertions(+), 78 deletions(-)
>
> diff --git a/clients/clickdot.c b/clients/clickdot.c
> index f52fbf04..f9e6e640 100644
> --- a/clients/clickdot.c
> +++ b/clients/clickdot.c
> @@ -32,9 +32,8 @@
> #include <cairo.h>
> #include <math.h>
> #include <assert.h>
> -#include <sys/timerfd.h>
> -#include <sys/epoll.h>
> #include <unistd.h>
> +#include <time.h>
>
> #include <linux/input.h>
> #include <wayland-client.h>
> @@ -62,8 +61,7 @@ struct clickdot {
> int reset;
>
> struct input *cursor_timeout_input;
> - int cursor_timeout_fd;
> - struct task cursor_timeout_task;
> + struct toytimer cursor_timeout;
> };
>
> static void
> @@ -224,14 +222,7 @@ button_handler(struct widget *widget,
> static void
> cursor_timeout_reset(struct clickdot *clickdot)
> {
> - const long cursor_timeout = 500;
> - struct itimerspec its;
> -
> - its.it_interval.tv_sec = 0;
> - its.it_interval.tv_nsec = 0;
> - its.it_value.tv_sec = cursor_timeout / 1000;
> - its.it_value.tv_nsec = (cursor_timeout % 1000) * 1000 * 1000;
> - timerfd_settime(clickdot->cursor_timeout_fd, 0, &its, NULL);
> + toytimer_arm_once_usec(&clickdot->cursor_timeout, 500 * 1000);
> }
>
> static int
> @@ -271,15 +262,10 @@ leave_handler(struct widget *widget,
> }
>
> static void
> -cursor_timeout_func(struct task *task, uint32_t events)
> +cursor_timeout_func(struct toytimer *tt)
> {
> struct clickdot *clickdot =
> - container_of(task, struct clickdot, cursor_timeout_task);
> - uint64_t exp;
> -
> - if (read(clickdot->cursor_timeout_fd, &exp, sizeof (uint64_t)) !=
> - sizeof(uint64_t))
> - abort();
> + container_of(tt, struct clickdot, cursor_timeout);
>
> input_set_pointer_image(clickdot->cursor_timeout_input,
> CURSOR_LEFT_PTR);
> @@ -317,12 +303,8 @@ clickdot_create(struct display *display)
> clickdot->line.old_y = -1;
> clickdot->reset = 0;
>
> - clickdot->cursor_timeout_fd =
> - timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
> - clickdot->cursor_timeout_task.run = cursor_timeout_func;
> - display_watch_fd(window_get_display(clickdot->window),
> - clickdot->cursor_timeout_fd,
> - EPOLLIN, &clickdot->cursor_timeout_task);
> + toytimer_init(&clickdot->cursor_timeout, CLOCK_MONOTONIC,
> + display, cursor_timeout_func);
>
> return clickdot;
> }
> @@ -330,9 +312,7 @@ clickdot_create(struct display *display)
> static void
> clickdot_destroy(struct clickdot *clickdot)
> {
> - display_unwatch_fd(window_get_display(clickdot->window),
> - clickdot->cursor_timeout_fd);
> - close(clickdot->cursor_timeout_fd);
> + toytimer_fini(&clickdot->cursor_timeout);
> if (clickdot->buffer)
> cairo_surface_destroy(clickdot->buffer);
> widget_destroy(clickdot->widget);
> diff --git a/clients/window.c b/clients/window.c
> index a94cee2c..e1c9de71 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -349,9 +349,8 @@ struct input {
> struct wl_callback *cursor_frame_cb;
> uint32_t cursor_timer_start;
> uint32_t cursor_anim_current;
> - int cursor_delay_fd;
> + struct toytimer cursor_timer;
> bool cursor_timer_running;
> - struct task cursor_task;
> struct wl_surface *pointer_surface;
> uint32_t modifiers;
> uint32_t pointer_enter_serial;
> @@ -440,8 +439,7 @@ struct tooltip {
> struct widget *parent;
> struct widget *widget;
> char *entry;
> - struct task tooltip_task;
> - int tooltip_fd;
> + struct toytimer timer;
> float x, y;
> };
>
> @@ -2122,21 +2120,17 @@ widget_destroy_tooltip(struct widget *parent)
> tooltip->widget = NULL;
> }
>
> - close(tooltip->tooltip_fd);
> + toytimer_fini(&tooltip->timer);
> free(tooltip->entry);
> free(tooltip);
> parent->tooltip = NULL;
> }
>
> static void
> -tooltip_func(struct task *task, uint32_t events)
> +tooltip_func(struct toytimer *tt)
> {
> - struct tooltip *tooltip =
> - container_of(task, struct tooltip, tooltip_task);
> - uint64_t exp;
> + struct tooltip *tooltip = container_of(tt, struct tooltip, timer);
>
> - if (read(tooltip->tooltip_fd, &exp, sizeof (uint64_t)) != sizeof (uint64_t))
> - abort();
> window_create_tooltip(tooltip);
> }
>
> @@ -2144,16 +2138,7 @@ tooltip_func(struct task *task, uint32_t events)
> static int
> tooltip_timer_reset(struct tooltip *tooltip)
> {
> - struct itimerspec its;
> -
> - its.it_interval.tv_sec = 0;
> - its.it_interval.tv_nsec = 0;
> - its.it_value.tv_sec = TOOLTIP_TIMEOUT / 1000;
> - its.it_value.tv_nsec = (TOOLTIP_TIMEOUT % 1000) * 1000 * 1000;
> - if (timerfd_settime(tooltip->tooltip_fd, 0, &its, NULL) < 0) {
> - fprintf(stderr, "could not set timerfd\n: %m");
> - return -1;
> - }
> + toytimer_arm_once_usec(&tooltip->timer, TOOLTIP_TIMEOUT * 1000);
>
> return 0;
> }
> @@ -2186,15 +2171,8 @@ widget_set_tooltip(struct widget *parent, char *entry, float x, float y)
> tooltip->x = x;
> tooltip->y = y;
> tooltip->entry = strdup(entry);
> - tooltip->tooltip_fd = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
> - if (tooltip->tooltip_fd < 0) {
> - fprintf(stderr, "could not create timerfd\n: %m");
> - return -1;
> - }
> -
> - tooltip->tooltip_task.run = tooltip_func;
> - display_watch_fd(parent->window->display, tooltip->tooltip_fd,
> - EPOLLIN, &tooltip->tooltip_task);
> + toytimer_init(&tooltip->timer, CLOCK_MONOTONIC,
> + parent->window->display, tooltip_func);
> tooltip_timer_reset(tooltip);
>
> return 0;
> @@ -2685,19 +2663,12 @@ input_ungrab(struct input *input)
> static void
> cursor_delay_timer_reset(struct input *input, uint32_t duration)
> {
> - struct itimerspec its;
> -
> if (!duration)
> input->cursor_timer_running = false;
> else
> input->cursor_timer_running = true;
>
> - its.it_interval.tv_sec = 0;
> - its.it_interval.tv_nsec = 0;
> - its.it_value.tv_sec = duration / 1000;
> - its.it_value.tv_nsec = (duration % 1000) * 1000 * 1000;
> - if (timerfd_settime(input->cursor_delay_fd, 0, &its, NULL) < 0)
> - fprintf(stderr, "could not set cursor timerfd\n: %m");
> + toytimer_arm_once_usec(&input->cursor_timer, duration * 1000);
> }
>
> static void cancel_pointer_image_update(struct input *input)
> @@ -3888,20 +3859,16 @@ pointer_surface_frame_callback(void *data, struct wl_callback *callback,
> }
>
> static void
> -cursor_timer_func(struct task *task, uint32_t events)
> +cursor_timer_func(struct toytimer *tt)
> {
> - struct input *input = container_of(task, struct input, cursor_task);
> + struct input *input = container_of(tt, struct input, cursor_timer);
> struct timespec tp;
> struct wl_cursor *cursor;
> uint32_t time;
> - uint64_t exp;
>
> if (!input->cursor_timer_running)
> return;
>
> - if (read(input->cursor_delay_fd, &exp, sizeof (uint64_t)) != sizeof (uint64_t))
> - return;
> -
> cursor = input->display->cursors[input->current_cursor];
> if (!cursor)
> return;
> @@ -5876,12 +5843,9 @@ display_add_input(struct display *d, uint32_t id, int display_seat_version)
> }
>
> input->pointer_surface = wl_compositor_create_surface(d->compositor);
> - input->cursor_task.run = cursor_timer_func;
>
> - input->cursor_delay_fd = timerfd_create(CLOCK_MONOTONIC,
> - TFD_CLOEXEC | TFD_NONBLOCK);
> - display_watch_fd(d, input->cursor_delay_fd, EPOLLIN,
> - &input->cursor_task);
> + toytimer_init(&input->cursor_timer, CLOCK_MONOTONIC, d,
> + cursor_timer_func);
> set_repeat_info(input, 40, 400);
>
> input->repeat_timer_fd = timerfd_create(CLOCK_MONOTONIC,
> @@ -5932,7 +5896,7 @@ input_destroy(struct input *input)
> wl_list_remove(&input->link);
> wl_seat_destroy(input->seat);
> close(input->repeat_timer_fd);
> - close(input->cursor_delay_fd);
> + toytimer_fini(&input->cursor_timer);
> free(input);
> }
>
> @@ -6562,3 +6526,89 @@ keysym_modifiers_get_mask(struct wl_array *modifiers_map,
>
> return 1 << index;
> }
> +
> +static void
> +toytimer_fire(struct task *tsk, uint32_t events)
> +{
> + uint64_t e;
> + struct toytimer *tt;
> +
> + tt = container_of(tsk, struct toytimer, tsk);
> +
> + if (events != EPOLLIN)
> + fprintf(stderr, "unexpected timerfd events %x\n", events);
> +
> + if (!(events & EPOLLIN))
> + return;
> +
> + if (read(tt->fd, &e, sizeof e) != sizeof e) {
> + /* If we change the timer between the fd becoming
> + * readable and getting here, there'll be nothing to
> + * read and we get EAGAIN. */
> + if (errno != EAGAIN)
> + fprintf(stderr, "timer read failed: %m\n");
> + return;
> + }
> +
> + tt->callback(tt);
> +}
> +
> +void
> +toytimer_init(struct toytimer *tt, clockid_t clock, struct display *display,
> + toytimer_cb callback)
> +{
> + memset(tt, 0, sizeof *tt);
> +
> + tt->fd = timerfd_create(clock, TFD_CLOEXEC | TFD_NONBLOCK);
> + if (tt->fd == -1) {
> + fprintf(stderr, "creating timer failed: %m\n");
> + abort();
> + }
> +
> + tt->display = display;
> + tt->callback = callback;
> + tt->tsk.run = toytimer_fire;
> + display_watch_fd(display, tt->fd, EPOLLIN, &tt->tsk);
> +}
> +
> +void
> +toytimer_fini(struct toytimer *tt)
> +{
> + display_unwatch_fd(tt->display, tt->fd);
> + close(tt->fd);
> + tt->fd = -1;
> +}
> +
> +void
> +toytimer_arm(struct toytimer *tt, const struct itimerspec *its)
> +{
> + int ret;
> +
> + ret = timerfd_settime(tt->fd, 0, its, NULL);
> + if (ret < 0) {
> + fprintf(stderr, "timer setup failed: %m\n");
> + abort();
> + }
> +}
> +
> +#define USEC_PER_SEC 1000000
> +
> +void
> +toytimer_arm_once_usec(struct toytimer *tt, uint32_t usec)
> +{
> + struct itimerspec its;
> +
> + its.it_interval.tv_sec = 0;
> + its.it_interval.tv_nsec = 0;
> + its.it_value.tv_sec = usec / USEC_PER_SEC;
> + its.it_value.tv_nsec = (usec % USEC_PER_SEC) * 1000;
> + toytimer_arm(tt, &its);
> +}
> +
> +void
> +toytimer_disarm(struct toytimer *tt)
> +{
> + struct itimerspec its = {};
> +
> + toytimer_arm(tt, &its);
> +}
> diff --git a/clients/window.h b/clients/window.h
> index 282dd76f..fde5c2f0 100644
> --- a/clients/window.h
> +++ b/clients/window.h
> @@ -27,6 +27,7 @@
> #include "config.h"
>
> #include <stdint.h>
> +#include <time.h>
> #include <xkbcommon/xkbcommon.h>
> #include <wayland-client.h>
> #include <cairo.h>
> @@ -717,4 +718,30 @@ xkb_mod_mask_t
> keysym_modifiers_get_mask(struct wl_array *modifiers_map,
> const char *name);
>
> +struct toytimer;
> +typedef void (*toytimer_cb)(struct toytimer *);
> +
> +struct toytimer {
> + struct display *display;
> + struct task tsk;
> + int fd;
> + toytimer_cb callback;
> +};
> +
> +void
> +toytimer_init(struct toytimer *tt, clockid_t clock, struct display *display,
> + toytimer_cb callback);
> +
> +void
> +toytimer_fini(struct toytimer *tt);
> +
> +void
> +toytimer_arm(struct toytimer *tt, const struct itimerspec *its);
> +
> +void
> +toytimer_arm_once_usec(struct toytimer *tt, uint32_t usec);
> +
> +void
> +toytimer_disarm(struct toytimer *tt);
> +
> #endif
>
More information about the wayland-devel
mailing list