[PATCH] eventloop: clarify post_dispatch_check()

[Pekka Paalanen]

On Fri, 25 Aug 2017 12:23:32 +1000
chris at cooperteam.net wrote:

> From: Christopher James Halse Rogers <christopher.halse.rogers at canonical.com>
> 
> This *technically* changes the semantics of the return value of the source callbacks.
> Previously you could return a negative number from a source callback and it would prevent
> *other* source callbacks from triggering a subsequent recheck.
> 
> Doing that seems like such a bad idea it's not worth supporting.
> 
> Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers at canonical.com>
> ---
>  src/event-loop.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/src/event-loop.c b/src/event-loop.c
> index 6130d2a..7d4c08d 100644
> --- a/src/event-loop.c
> +++ b/src/event-loop.c
> @@ -29,6 +29,7 @@
>  #include <signal.h>
>  #include <stdlib.h>
>  #include <stdint.h>
> +#include <stdbool.h>
>  #include <string.h>
>  #include <fcntl.h>
>  #include <sys/socket.h>
> @@ -376,19 +377,18 @@ wl_event_loop_destroy(struct wl_event_loop *loop)
>  	free(loop);
>  }
>  
> -static int
> +static bool
>  post_dispatch_check(struct wl_event_loop *loop)
>  {
>  	struct epoll_event ep;
>  	struct wl_event_source *source, *next;
> -	int n;
> +	bool needs_recheck = false;
>  
>  	ep.events = 0;
> -	n = 0;
>  	wl_list_for_each_safe(source, next, &loop->check_list, link)
> -		n += source->interface->dispatch(source, &ep);
> +		needs_recheck |= source->interface->dispatch(source, &ep) != 0;
>  
> -	return n;
> +	return needs_recheck;
>  }
>  
>  WL_EXPORT void
> @@ -409,7 +409,7 @@ wl_event_loop_dispatch(struct wl_event_loop *loop, int timeout)
>  {
>  	struct epoll_event ep[32];
>  	struct wl_event_source *source;
> -	int i, count, n;
> +	int i, count;
>  
>  	wl_event_loop_dispatch_idle(loop);
>  
> @@ -427,9 +427,7 @@ wl_event_loop_dispatch(struct wl_event_loop *loop, int timeout)
>  
>  	wl_event_loop_dispatch_idle(loop);
>  
> -	do {
> -		n = post_dispatch_check(loop);
> -	} while (n > 0);
> +	while (post_dispatch_check(loop));
>  
>  	return 0;
>  }

Hi,

ooh, nasty indeed. A good find.

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

The only thing I wonder if we should add a check for a negative return
from ->dispatch() and log a warning in that case. In the unfortunate
event if someone actually used a negative value, on purpose or by
accident, the behaviour change should not be silent. OTOH, I think
wl_abort() would be too rough to add in hindsight.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170825/abe57372/attachment-0001.sig>