[PATCH] RFC: server: implement wl_priv_signal_emit without emit_list

[Pekka Paalanen]

On Sun, 28 Jan 2018 12:37:36 -0500
Simon Ser <contact at emersion.fr> wrote:

> This is a RFC to be able to run tests and check that this approach
> is working. The end goal is to remove wl_priv_signal completely and
> implement a safe wl_signal_emit.
> ---
> What do you think of this approach? It passes all the tests.
>  src/wayland-private.h |  1 -
>  src/wayland-server.c  | 31 ++++++++++++++++---------------
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/src/wayland-private.h b/src/wayland-private.h
> index 12b9032..ad58226 100644
> --- a/src/wayland-private.h
> +++ b/src/wayland-private.h
> @@ -238,7 +238,6 @@ zalloc(size_t s)
>  
>  struct wl_priv_signal {
>  	struct wl_list listener_list;
> -	struct wl_list emit_list;
>  };
>  
>  void
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 7225b4e..d587816 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1912,7 +1912,6 @@ void
>  wl_priv_signal_init(struct wl_priv_signal *signal)
>  {
>  	wl_list_init(&signal->listener_list);
> -	wl_list_init(&signal->emit_list);
>  }
>  
>  /** Add a listener to a signal
> @@ -1947,9 +1946,6 @@ wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify)
>  	wl_list_for_each(l, &signal->listener_list, link)
>  		if (l->notify == notify)
>  			return l;
> -	wl_list_for_each(l, &signal->emit_list, link)
> -		if (l->notify == notify)
> -			return l;
>  
>  	return NULL;
>  }
> @@ -1966,25 +1962,30 @@ wl_priv_signal_emit(struct wl_priv_signal *signal, void *data)
>  {
>  	struct wl_listener *l;
>  	struct wl_list *pos;
> +	struct wl_listener cursor;
> +	struct wl_listener end;
>  
> -	wl_list_insert_list(&signal->emit_list, &signal->listener_list);
> -	wl_list_init(&signal->listener_list);
> -
> -	/* Take every element out of the list and put them in a temporary list.
> -	 * This way, the 'it' func can remove any element it wants from the list
> -	 * without troubles, because we always get the first element, not the
> -	 * one after the current, which may be invalid.
> +	/* Add two special markers: one cursor and one end marker. This way, we know
> +	 * that we've already called listeners on the left of the cursor and that we
> +	 * don't want to call listeners on the right of the end marker. The 'it'
> +	 * function can remove any element it wants from the list without troubles.
>  	 * wl_list_for_each_safe tries to be safe but it fails: it works fine
>  	 * if the current item is removed, but not if the next one is. */
> -	while (!wl_list_empty(&signal->emit_list)) {
> -		pos = signal->emit_list.next;
> +	wl_list_insert(&signal->listener_list, &cursor.link);
> +	wl_list_insert(signal->listener_list.prev, &end.link);
> +
> +	while (cursor.link.next != &end.link) {
> +		pos = cursor.link.next;
>  		l = wl_container_of(pos, l, link);
>  
> -		wl_list_remove(pos);
> -		wl_list_insert(&signal->listener_list, pos);
> +		wl_list_remove(&cursor.link);
> +		wl_list_insert(pos, &cursor.link);
>  
>  		l->notify(l, data);
>  	}
> +
> +	wl_list_remove(&cursor.link);
> +	wl_list_remove(&end.link);
>  }
>  
>  /** \endcond INTERNAL */

Hi Simon,

that's a really original approach to the problem. After actually
reading it, I cannot come up with any case where it would fail. Brain
is still to hazy to give a R-b, so I'll just give a

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

It is true that wl_listener is almost always sub-classed, but then the
"dynamic_cast" requires checking the object type from the 'notify'
member, so there should be no surprises with object types.


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/20180214/a882362c/attachment.sig>