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

[Tom Gundersen]

On Thu, Aug 21, 2014 at 12:35 AM, Lennart Poettering
<lennart at poettering.net> wrote:
> 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...

Hm, so I would have liked to just return 0 instead of 1 for
EAGAIN/EINTR. This way sd_event_wait() only returns 1 if there are
pending events.

However, without further changes, this would mean that sd_event_run()
would return 0, rather than 1, in case the poll was interrupted (the
new semantics would be to return 1 only when events have been
dispatched, which makes sense to me). However, I'm not sure about the
rationale for the current semantics?

>> +_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;

Indeed, that's more readable.

> Looks good otherwise. I like it.
>
>
> Lennart
>
> --
> Lennart Poettering, Red Hat