[systemd-devel] [PATCH 1/2] [RFC] sd-event: split run into prepare/wait/dispatch

[Lennart Poettering]

On Wed, 20.08.14 17:28, Tom Gundersen (teg at jklm.no) wrote:

> +
> +_public_ int sd_event_wait(sd_event *e, uint64_t timeout) {
> +        struct epoll_event *ev_queue;
> +        unsigned ev_queue_max;
> +        int r, m, i;
> +
> +        assert_return(e, -EINVAL);
> +        assert_return(!event_pid_changed(e), -ECHILD);
> +        assert_return(e->state != SD_EVENT_FINISHED, -ESTALE);
> +        assert_return(e->state == SD_EVENT_PREPARED, -EBUSY);
> +
> +        if (e->exit_requested) {
> +                e->state = SD_EVENT_PENDING;
> +                return 1;
> +        }
>  
>          ev_queue_max = CLAMP(e->n_sources, 1U, EPOLL_QUEUE_MAX);
>          ev_queue = newa(struct epoll_event, ev_queue_max);
> @@ -2262,12 +2280,10 @@ _public_ int sd_event_run(sd_event *e, uint64_t timeout) {
>          m = epoll_wait(e->epoll_fd, ev_queue, ev_queue_max,
>                         timeout == (uint64_t) -1 ? -1 : (int) ((timeout + USEC_PER_MSEC - 1) / USEC_PER_MSEC));
>          if (m < 0) {
> -                r = errno == EAGAIN || errno == EINTR ? 1 : -errno;
> +                r = -errno;

Why this change? We should still eat up EAGAIN/EINTR, they aren't really errors...

> +_public_ int sd_event_run(sd_event *e, uint64_t timeout) {
> +        int r;
> +
> +        assert_return(e, -EINVAL);
> +        assert_return(!event_pid_changed(e), -ECHILD);
> +        assert_return(e->state != SD_EVENT_FINISHED, -ESTALE);
> +        assert_return(e->state == SD_EVENT_PASSIVE, -EBUSY);
> +
> +        r = sd_event_prepare(e);
> +        if (r > 0)
> +                return sd_event_dispatch(e);
> +        if (r == 0) {
> +                r = sd_event_wait(e, timeout);
> +                if (r > 0)
> +                        return sd_event_dispatch(e);
> +                else
> +                        return (r == -EAGAIN || r == -EINTR) ? 1 : r;
> +        } else
> +                return (r == -EAGAIN || r == -EINTR) ? 1 : r;
> +}


I think this would be nicer:

r = sd_event_prepare(...);
if (r < 0)
        return ...
if (r > 0)
        return ...

r = sd_event_wait(...)
if (r < 0)
        return ...
if (r > 0)
        return ...

return 0;

Looks good otherwise. I like it.


Lennart

-- 
Lennart Poettering, Red Hat