On 2023/02/09 17:19:25 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> Not a functional change. But while trying to figure out why gotd
> was crashing in libevent, this stood out as a potential suspect
> of libevent misuse.

it took me a bit to see that this is safe.  It will still delete the
event in disconnect().

it's certainly better than calling event_del in multiple places, but
this also means that we could be awakened to dispatch on gotd_request
multiple times for events that we're not interested in.  maybe we
could just re-initialize the event in connect_session?  event_del +
event_set (without event_add) should do it.

it's also not clear to me if we could still have something queued for
sending in connect_session.  msgbuf_clear will delete pending data.
could still be useful to end up in gotd_request to flush that data.

so, in short, i think this improves the current situation but not sure
if it does so in the right direction ^^".

> -----------------------------------------------
>  avoid calling event_del() for unrelated events in connect_session() 
>  
>  Rather, we can treat fd == -1 as a flag which tells gotd_request() to
>  stop re-adding the event for this client. This seems cleaner.
>  
> diff 90270f794c7f4e1ce2b58c8b5bfdc190df4799b6 a993d087b9f1bd1801784b5b8ab7f84cec9d5664
> commit - 90270f794c7f4e1ce2b58c8b5bfdc190df4799b6
> commit + a993d087b9f1bd1801784b5b8ab7f84cec9d5664
> blob - 00d1d81a42f9598ad0cba57a03f07fb868556714
> blob + 2d3fb7e54d9404bb25410b6225921529fff39e24
> --- gotd/gotd.c
> +++ gotd/gotd.c
> @@ -579,6 +579,10 @@ gotd_request(int fd, short events, void *arg)
>  	struct imsg imsg;
>  	ssize_t n;
>  
> +	/* Ignore events from clients which already have a session running. */
> +	if (client->fd == -1)
> +		return;
> +
>  	if (events & EV_WRITE) {
>  		while (ibuf->w.queued) {
>  			n = msgbuf_write(&ibuf->w);
> @@ -1197,10 +1201,11 @@ connect_session(struct gotd_client *client)
>  	/*
>  	 * We are no longer interested in messages from this client.
>  	 * Further client requests will be handled by the session process.
> +	 * Clear outgoing message queues and set client->fd to -1 which will
> +	 * cause gotd_request to ignore future messages from this client.
>  	 */
>  	msgbuf_clear(&client->iev.ibuf.w);
>  	imsg_clear(&client->iev.ibuf);
> -	event_del(&client->iev.ev);
>  	client->fd = -1; /* will be closed via copy in client->iev.ibuf.fd */
>  
>  	return NULL;