On Sun, 28 Jan 2018 12:37:36 -0500
Simon Ser <cont...@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.paala...@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

Attachment: pgpa7cjoNiCET.pgp
Description: OpenPGP digital signature

_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to