[systemd-devel] sd_event_run

[Tom Gundersen]

On Sat, Mar 14, 2015 at 2:41 AM, Zbigniew Jędrzejewski-Szmek
<zbyszek at in.waw.pl> wrote:
> On Fri, Mar 13, 2015 at 06:10:17PM +0100, Tom Gundersen wrote:
>> On Fri, Mar 13, 2015 at 4:35 PM, Zbigniew Jędrzejewski-Szmek
>> <zbyszek at in.waw.pl> wrote:
>> > you added sd_event_run a while ago:
>> >
>> > commit c45a5a74465a39280b855f9d720b2ab4779a47fa
>> > Author: Tom Gundersen <teg at jklm.no>
>> > Date:   Fri Aug 15 18:49:29 2014 +0200
>> >
>> >     sd-event: split run into prepare/wait/dispatch
>> >
>> >     This will allow sd-event to be integrated into an external event loop, which
>> >     in turn will allow (say) glib-based applications to use our various libraries,
>> >     without manually integrating each of them (bus, rtnl, dhcp, ...).
>> >
>> >     The external event-loop should integrate sd-event int he following way:
>> >
>> >     Every iteration must start with a call to sd_event_prepare(), which will
>> >     return 0 if no event sources are ready to be processed, a positive value if
>> >     they are and a negative value on error. sd_event_prepare() may only be called
>> >     following sd_event_dispatch(); a call to sd_event_wait() indicating that no
>> >     sources are ready to be dispatched; or a failed call to sd_event_dispatch() or
>> >     sd_event_wait().
>> >
>> >     A successful call to sd_event_prepare() indicating that no event sources are
>> >     ready to be dispatched must be followed by a call to sd_event_wait(),
>> >     which will return 0 if it timed out without event sources being ready to
>> >     be processed, a negative value on error and a positive value otherwise.
>> >     sd_event_wait() may only be called following a successful call to
>> >     sd_event_prepare() indicating that no event sources are ready to be dispatched.
>> >
>> >     If sd_event_wait() indicates that some events sources are ready to be
>> >     dispatched, it must be followed by a call to sd_event_dispatch(). This
>> >     is the only time sd_event_dispatch() may be called.
>> >
>> > +_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);
>> > +        else if (r < 0)
>> > +                return r;
>> > +
>> > +        r = sd_event_wait(e, timeout);
>> > +        if (r > 0)
>> > +                return sd_event_dispatch(e);
>> > +        else
>> > +                return r;
>> > +}
>> >
>> > Your commit description is almost ready to be turned into a man page, but there
>> > a hiccup. According to the last paragraph of the commit message, sd_event_dispatch may
>> > only be called after sd_event_wait(). This contradict the code in sd_event_run().
>> > (sd_event_dispatch calls sd_event_wait internally, but the user does not know
>> > this). Can you clarify the intended rules?
>>
>> Indeed. Perhaps the best way to explain it is to look at the states:
>>
>>         SD_EVENT_PASSIVE,
>>         SD_EVENT_PREPARED,
>>         SD_EVENT_PENDING,
>>         SD_EVENT_FINISHED,
> There's also SD_EVENT_RUNNING. It probably should be documented too,
> but I didn't include it in the description of
> sd_event_{run,wait,prepare,dispatch,loop}.

Yeah, there are other states, but not relevant when talking of these
functions I think.

> A draft is attached, please have a look. After writing it I had some thoughts:

Looks great.

> 1. shouldn't SD_EVENT_PASSIVE become SD_EVENT_INITIAL? "passive" seems strange
>    in this context.
>    Similarly, SD_EVENT_ARMED seems more self-explanatory than PREPARED.
>    (I don't like PREPARED because it is not obvious whether sources are
>    prepared to wait on, or events are prepared to be reaped.)

Yeah, I agree. I pushed the rename.

> 2. shouldn't sd_event_dispatch also return 0/1 to signify e.g.
>    loop-continues / loop-finished. Right now other functions return
>    the state as return value, but sd_event_dispatch requires querying
>    the event object for state.

Hm, it appears this is how it already works. 0 signifies
SD_EVENT_FINISHED and 1 SD_EVENT_INITIAL.

Currently sd_event_run() returns 0 on timeout, so we have to check
sd_event_get_state() to check if it finished. Maybe we should consider
returning -TIMEDOUT on timeout and 0 on FINISH instead?

Cheers,

Tom