ok beck@
On Fri, Sep 18, 2015 at 5:17 PM, Alexander Bluhm
<alexander.bl...@gmx.net> wrote:
> On Fri, Sep 18, 2015 at 05:04:55PM -0600, Bob Beck wrote:
>> ... and just to be clear, you only need to event_del when you are
>> switching to wanting a write when you did want a read before, and vice
>> versa... correct?
>
> Yes. When I am doing reads and get a TLS_WANT_POLLOUT, I have to
> do a event_set() on the write event to change the callback to read.
> As there might be an event scheduled on the event, I have to do an
> event_del() before event_set().
>
> That's why the changing case is a bit more complex.
>
> After success I have to change back the other event, so I need
> event_del() and event_set() again.
>
> bluhm
>
>> if that's the case it reads ok...
>>
>>
>> On Fri, Sep 18, 2015 at 4:55 PM, Alexander Bluhm
>> <alexander.bl...@gmx.net> wrote:
>> > Hi,
>> >
>> > I discovered what caused the strange event loss in syslogd during
>> > the hackaton. I had mixed EV_READ and EV_WRITE events on the ev_read
>> > and ev_write event structures. The correct way is to use each event
>> > for its read and write purpose and instead switch the handler. Then
>> > libevent is no longer confused.
>> >
>> > ok?
>> >
>> > bluhm
>> >
>> > Index: usr.sbin/syslogd/evbuffer_tls.c
>> > ===================================================================
>> > RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/evbuffer_tls.c,v
>> > retrieving revision 1.7
>> > diff -u -p -r1.7 evbuffer_tls.c
>> > --- usr.sbin/syslogd/evbuffer_tls.c 10 Sep 2015 18:32:06 -0000 1.7
>> > +++ usr.sbin/syslogd/evbuffer_tls.c 18 Sep 2015 22:53:53 -0000
>> > @@ -44,6 +44,9 @@
>> > /* prototypes */
>> >
>> > void bufferevent_read_pressure_cb(struct evbuffer *, size_t, size_t, void
>> > *);
>> > +static void buffertls_readcb(int, short, void *);
>> > +static void buffertls_writecb(int, short, void *);
>> > +static void buffertls_handshakecb(int, short, void *);
>> > int evtls_read(struct evbuffer *, int, int, struct tls *);
>> > int evtls_write(struct evbuffer *, int, struct tls *);
>> >
>> > @@ -96,29 +99,30 @@ buffertls_readcb(int fd, short event, vo
>> > res = evtls_read(bufev->input, fd, howmuch, ctx);
>> > switch (res) {
>> > case TLS_WANT_POLLIN:
>> > - event_set(&bufev->ev_read, fd, EV_READ, buffertls_readcb,
>> > - buftls);
>> > - goto reschedule;
>> > + bufferevent_add(&bufev->ev_read, bufev->timeout_read);
>> > + return;
>> > case TLS_WANT_POLLOUT:
>> > - event_set(&bufev->ev_read, fd, EV_WRITE, buffertls_readcb,
>> > + event_del(&bufev->ev_write);
>> > + event_set(&bufev->ev_write, fd, EV_WRITE, buffertls_readcb,
>> > buftls);
>> > - goto reschedule;
>> > + bufferevent_add(&bufev->ev_write, bufev->timeout_write);
>> > + return;
>> > case -1:
>> > - if (errno == EAGAIN || errno == EINTR)
>> > - goto reschedule;
>> > - /* error case */
>> > what |= EVBUFFER_ERROR;
>> > break;
>> > case 0:
>> > - /* eof case */
>> > what |= EVBUFFER_EOF;
>> > break;
>> > }
>> > if (res <= 0)
>> > goto error;
>> >
>> > - event_set(&bufev->ev_read, fd, EV_READ, buffertls_readcb, buftls);
>> > - bufferevent_add(&bufev->ev_read, bufev->timeout_read);
>> > + event_del(&bufev->ev_write);
>> > + event_set(&bufev->ev_write, fd, EV_WRITE, buffertls_writecb,
>> > buftls);
>> > + if (bufev->enabled & EV_READ)
>> > + bufferevent_add(&bufev->ev_read, bufev->timeout_read);
>> > + if (EVBUFFER_LENGTH(bufev->output) != 0 && bufev->enabled &
>> > EV_WRITE)
>> > + bufferevent_add(&bufev->ev_write, bufev->timeout_write);
>> >
>> > /* See if this callbacks meets the water marks */
>> > len = EVBUFFER_LENGTH(bufev->input);
>> > @@ -137,10 +141,6 @@ buffertls_readcb(int fd, short event, vo
>> > (*bufev->readcb)(bufev, bufev->cbarg);
>> > return;
>> >
>> > - reschedule:
>> > - bufferevent_add(&bufev->ev_read, bufev->timeout_read);
>> > - return;
>> > -
>> > error:
>> > (*bufev->errorcb)(bufev, what, bufev->cbarg);
>> > }
>> > @@ -163,22 +163,18 @@ buffertls_writecb(int fd, short event, v
>> > res = evtls_write(bufev->output, fd, ctx);
>> > switch (res) {
>> > case TLS_WANT_POLLIN:
>> > - event_set(&bufev->ev_write, fd, EV_READ,
>> > + event_del(&bufev->ev_read);
>> > + event_set(&bufev->ev_read, fd, EV_READ,
>> > buffertls_writecb, buftls);
>> > - goto reschedule;
>> > + bufferevent_add(&bufev->ev_read,
>> > bufev->timeout_read);
>> > + return;
>> > case TLS_WANT_POLLOUT:
>> > - event_set(&bufev->ev_write, fd, EV_WRITE,
>> > - buffertls_writecb, buftls);
>> > - goto reschedule;
>> > + bufferevent_add(&bufev->ev_write,
>> > bufev->timeout_write);
>> > + return;
>> > case -1:
>> > - if (errno == EAGAIN || errno == EINTR ||
>> > - errno == EINPROGRESS)
>> > - goto reschedule;
>> > - /* error case */
>> > what |= EVBUFFER_ERROR;
>> > break;
>> > case 0:
>> > - /* eof case */
>> > what |= EVBUFFER_EOF;
>> > break;
>> > }
>> > @@ -186,8 +182,11 @@ buffertls_writecb(int fd, short event, v
>> > goto error;
>> > }
>> >
>> > - event_set(&bufev->ev_write, fd, EV_WRITE, buffertls_writecb,
>> > buftls);
>> > - if (EVBUFFER_LENGTH(bufev->output) != 0)
>> > + event_del(&bufev->ev_read);
>> > + event_set(&bufev->ev_read, fd, EV_READ, buffertls_readcb, buftls);
>> > + if (bufev->enabled & EV_READ)
>> > + bufferevent_add(&bufev->ev_read, bufev->timeout_read);
>> > + if (EVBUFFER_LENGTH(bufev->output) != 0 && bufev->enabled &
>> > EV_WRITE)
>> > bufferevent_add(&bufev->ev_write, bufev->timeout_write);
>> >
>> > /*
>> > @@ -200,11 +199,6 @@ buffertls_writecb(int fd, short event, v
>> >
>> > return;
>> >
>> > - reschedule:
>> > - if (EVBUFFER_LENGTH(bufev->output) != 0)
>> > - bufferevent_add(&bufev->ev_write, bufev->timeout_write);
>> > - return;
>> > -
>> > error:
>> > (*bufev->errorcb)(bufev, what, bufev->cbarg);
>> > }
>> > @@ -226,41 +220,30 @@ buffertls_handshakecb(int fd, short even
>> > res = tls_handshake(ctx);
>> > switch (res) {
>> > case TLS_WANT_POLLIN:
>> > - event_set(&bufev->ev_write, fd, EV_READ,
>> > - buffertls_handshakecb, buftls);
>> > - goto reschedule;
>> > + bufferevent_add(&bufev->ev_read, bufev->timeout_read);
>> > + return;
>> > case TLS_WANT_POLLOUT:
>> > - event_set(&bufev->ev_write, fd, EV_WRITE,
>> > - buffertls_handshakecb, buftls);
>> > - goto reschedule;
>> > + bufferevent_add(&bufev->ev_write, bufev->timeout_write);
>> > + return;
>> > case -1:
>> > - if (errno == EAGAIN || errno == EINTR ||
>> > - errno == EINPROGRESS)
>> > - goto reschedule;
>> > - /* error case */
>> > what |= EVBUFFER_ERROR;
>> > break;
>> > }
>> > if (res < 0)
>> > goto error;
>> >
>> > - /*
>> > - * There might be data available in the tls layer. Try
>> > - * an read operation and setup the callbacks. Call the read
>> > - * callback after enabling the write callback to give the
>> > - * read error handler a chance to disable the write event.
>> > - */
>> > + /* Handshake was successful, change to read and write callback. */
>> > + event_del(&bufev->ev_read);
>> > + event_del(&bufev->ev_write);
>> > + event_set(&bufev->ev_read, fd, EV_READ, buffertls_readcb, buftls);
>> > event_set(&bufev->ev_write, fd, EV_WRITE, buffertls_writecb,
>> > buftls);
>> > - if (EVBUFFER_LENGTH(bufev->output) != 0)
>> > + if (bufev->enabled & EV_READ)
>> > + bufferevent_add(&bufev->ev_read, bufev->timeout_read);
>> > + if (EVBUFFER_LENGTH(bufev->output) != 0 && bufev->enabled &
>> > EV_WRITE)
>> > bufferevent_add(&bufev->ev_write, bufev->timeout_write);
>> > - buffertls_readcb(fd, 0, buftls);
>> >
>> > return;
>> >
>> > - reschedule:
>> > - bufferevent_add(&bufev->ev_write, bufev->timeout_write);
>> > - return;
>> > -
>> > error:
>> > (*bufev->errorcb)(bufev, what, bufev->cbarg);
>> > }
>> > @@ -283,7 +266,7 @@ buffertls_connect(struct buffertls *buft
>> >
>> > event_del(&bufev->ev_read);
>> > event_del(&bufev->ev_write);
>> > -
>> > + event_set(&bufev->ev_read, fd, EV_READ, buffertls_handshakecb,
>> > buftls);
>> > event_set(&bufev->ev_write, fd, EV_WRITE, buffertls_handshakecb,
>> > buftls);
>> > bufferevent_add(&bufev->ev_write, bufev->timeout_write);
>> >
