On 2018-03-09 05:44 AM, Pekka Paalanen wrote:
From: Pekka Paalanen <[email protected]>

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 <[email protected]>

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 <[email protected]>
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


_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to