On Thu, Aug 21, 2014 at 12:35 AM, Lennart Poettering
<lenn...@poettering.net> wrote:
> On Wed, 20.08.14 17:28, Tom Gundersen (t...@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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
