[PATCH] core: implement a safe wl_signal_emit
Derek Foreman
derekf at osg.samsung.com
Wed Feb 21 17:21:59 UTC 2018
On 2018-02-14 09:23 AM, Simon Ser wrote:
> The previous implementation tried to be safe but wasn't: a
> listener couldn't remove the next one.
>
> This removes the need for wl_priv_signal. newsignal-test is now
> merged into signal-test.
> ---
> This is followup of "RFC: server: implement wl_priv_signal_emit without
> emit_list" [1] which removes wl_priv_signal.
Thanks for this, but I think there's a serious problem with this patch
as-is...
Currently, weston, enlightenment, and mutter don't unlink their buffer
destroy handler's notifier from the destroy handler before freeing the
memory the listener link is stored in.
Since a destroy signal inidicates the object is utterly dead, I don't
think it's unreasonable for users to have assumed that they don't have
to clean up their listener link. It's *never* going to fire again, so
why should anything need to be removed?
It seems that this patch makes that assumption invalid, and we would
need patches to weston, enlightenment, and mutter to prevent a
use-after-free during the signal emit? Now I'm seeing valgrind errors
on E and weston during buffer destroy.
Personally, I don't think we should change this assumption and declare
the existing code that's worked for years suddenly buggy. :/
I'm afraid I want to NAK this one - I still like the idea of removing
wl_priv_signal_* and enabling your delete-other-list-item use case, but
to move forward, I think we need an implementation that doesn't break
the old assumptions.
Thanks,
Derek
> [1]: https://lists.freedesktop.org/archives/wayland-devel/2018-January/036711.html
> Makefile.am | 3 -
> src/wayland-private.h | 17 ---
> src/wayland-server-core.h | 28 +++-
> src/wayland-server.c | 141 ++++---------------
> tests/newsignal-test.c | 337 ----------------------------------------------
> tests/signal-test.c | 222 +++++++++++++++++++++++++++++-
> 6 files changed, 270 insertions(+), 478 deletions(-)
> delete mode 100644 tests/newsignal-test.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 322d6b8..5db0347 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -171,7 +171,6 @@ built_test_programs = \
> socket-test \
> queue-test \
> signal-test \
> - newsignal-test \
> resources-test \
> message-test \
> headers-test \
> @@ -252,8 +251,6 @@ queue_test_LDADD = libtest-runner.la
> signal_test_SOURCES = tests/signal-test.c
> signal_test_LDADD = libtest-runner.la
> # wayland-server.c is needed here to access wl_priv_* functions
> -newsignal_test_SOURCES = tests/newsignal-test.c src/wayland-server.c
> -newsignal_test_LDADD = libtest-runner.la
> resources_test_SOURCES = tests/resources-test.c
> resources_test_LDADD = libtest-runner.la
> message_test_SOURCES = tests/message-test.c
> diff --git a/src/wayland-private.h b/src/wayland-private.h
> index 12b9032..e2803ee 100644
> --- a/src/wayland-private.h
> +++ b/src/wayland-private.h
> @@ -236,23 +236,6 @@ zalloc(size_t s)
> return calloc(1, s);
> }
>
> -struct wl_priv_signal {
> - struct wl_list listener_list;
> - struct wl_list emit_list;
> -};
> -
> -void
> -wl_priv_signal_init(struct wl_priv_signal *signal);
> -
> -void
> -wl_priv_signal_add(struct wl_priv_signal *signal, struct wl_listener *listener);
> -
> -struct wl_listener *
> -wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify);
> -
> -void
> -wl_priv_signal_emit(struct wl_priv_signal *signal, void *data);
> -
> void
> wl_connection_close_fds_in(struct wl_connection *connection, int max);
>
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index 2e725d9..965bb39 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -462,10 +462,34 @@ wl_signal_get(struct wl_signal *signal, wl_notify_func_t notify)
> static inline void
> wl_signal_emit(struct wl_signal *signal, void *data)
> {
> - struct wl_listener *l, *next;
> + struct wl_listener *l;
> + struct wl_list *pos;
> + struct wl_listener cursor;
> + struct wl_listener end;
> +
> + /* 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. */
> + wl_list_insert(&signal->listener_list, &cursor.link);
> + cursor.notify = NULL;
> + wl_list_insert(signal->listener_list.prev, &end.link);
> + end.notify = NULL;
> +
> + while (cursor.link.next != &end.link) {
> + pos = cursor.link.next;
> + l = wl_container_of(pos, l, link);
> +
> + wl_list_remove(&cursor.link);
> + wl_list_insert(pos, &cursor.link);
>
> - wl_list_for_each_safe(l, next, &signal->listener_list, link)
> l->notify(l, data);
> + }
> +
> + wl_list_remove(&cursor.link);
> + wl_list_remove(&end.link);
> }
>
> typedef void (*wl_resource_destroy_func_t)(struct wl_resource *resource);
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index eb1e500..c41f8e9 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -76,10 +76,10 @@ struct wl_client {
> struct wl_resource *display_resource;
> struct wl_list link;
> struct wl_map objects;
> - struct wl_priv_signal destroy_signal;
> + struct wl_signal destroy_signal;
> struct ucred ucred;
> int error;
> - struct wl_priv_signal resource_created_signal;
> + struct wl_signal resource_created_signal;
> };
>
> struct wl_display {
> @@ -95,8 +95,8 @@ struct wl_display {
> struct wl_list client_list;
> struct wl_list protocol_loggers;
>
> - struct wl_priv_signal destroy_signal;
> - struct wl_priv_signal create_client_signal;
> + struct wl_signal destroy_signal;
> + struct wl_signal create_client_signal;
>
> struct wl_array additional_shm_formats;
>
> @@ -127,7 +127,7 @@ struct wl_resource {
> void *data;
> int version;
> wl_dispatcher_func_t dispatcher;
> - struct wl_priv_signal destroy_signal;
> + struct wl_signal destroy_signal;
> };
>
> struct wl_protocol_logger {
> @@ -508,7 +508,7 @@ wl_client_create(struct wl_display *display, int fd)
> if (client == NULL)
> return NULL;
>
> - wl_priv_signal_init(&client->resource_created_signal);
> + wl_signal_init(&client->resource_created_signal);
> client->display = display;
> client->source = wl_event_loop_add_fd(display->loop, fd,
> WL_EVENT_READABLE,
> @@ -531,13 +531,13 @@ wl_client_create(struct wl_display *display, int fd)
> if (wl_map_insert_at(&client->objects, 0, 0, NULL) < 0)
> goto err_map;
>
> - wl_priv_signal_init(&client->destroy_signal);
> + wl_signal_init(&client->destroy_signal);
> if (bind_display(client, display) < 0)
> goto err_map;
>
> wl_list_insert(display->client_list.prev, &client->link);
>
> - wl_priv_signal_emit(&display->create_client_signal, client);
> + wl_signal_emit(&display->create_client_signal, client);
>
> return client;
>
> @@ -682,7 +682,7 @@ destroy_resource(void *element, void *data, uint32_t flags)
> /* Don't emit the new signal for deprecated resources, as that would
> * access memory outside the bounds of the deprecated struct */
> if (!resource_is_deprecated(resource))
> - wl_priv_signal_emit(&resource->destroy_signal, resource);
> + wl_signal_emit(&resource->destroy_signal, resource);
>
> if (resource->destroy)
> resource->destroy(resource);
> @@ -798,7 +798,7 @@ wl_resource_add_destroy_listener(struct wl_resource *resource,
> if (resource_is_deprecated(resource))
> wl_signal_add(&resource->deprecated_destroy_signal, listener);
> else
> - wl_priv_signal_add(&resource->destroy_signal, listener);
> + wl_signal_add(&resource->destroy_signal, listener);
> }
>
> WL_EXPORT struct wl_listener *
> @@ -807,7 +807,7 @@ wl_resource_get_destroy_listener(struct wl_resource *resource,
> {
> if (resource_is_deprecated(resource))
> return wl_signal_get(&resource->deprecated_destroy_signal, notify);
> - return wl_priv_signal_get(&resource->destroy_signal, notify);
> + return wl_signal_get(&resource->destroy_signal, notify);
> }
>
> /** Retrieve the interface name (class) of a resource object.
> @@ -826,14 +826,14 @@ WL_EXPORT void
> wl_client_add_destroy_listener(struct wl_client *client,
> struct wl_listener *listener)
> {
> - wl_priv_signal_add(&client->destroy_signal, listener);
> + wl_signal_add(&client->destroy_signal, listener);
> }
>
> WL_EXPORT struct wl_listener *
> wl_client_get_destroy_listener(struct wl_client *client,
> wl_notify_func_t notify)
> {
> - return wl_priv_signal_get(&client->destroy_signal, notify);
> + return wl_signal_get(&client->destroy_signal, notify);
> }
>
> WL_EXPORT void
> @@ -841,7 +841,7 @@ wl_client_destroy(struct wl_client *client)
> {
> uint32_t serial = 0;
>
> - wl_priv_signal_emit(&client->destroy_signal, client);
> + wl_signal_emit(&client->destroy_signal, client);
>
> wl_client_flush(client);
> wl_map_for_each(&client->objects, destroy_resource, &serial);
> @@ -1023,8 +1023,8 @@ wl_display_create(void)
> wl_list_init(&display->registry_resource_list);
> wl_list_init(&display->protocol_loggers);
>
> - wl_priv_signal_init(&display->destroy_signal);
> - wl_priv_signal_init(&display->create_client_signal);
> + wl_signal_init(&display->destroy_signal);
> + wl_signal_init(&display->create_client_signal);
>
> display->id = 1;
> display->serial = 0;
> @@ -1089,7 +1089,7 @@ wl_display_destroy(struct wl_display *display)
> struct wl_socket *s, *next;
> struct wl_global *global, *gnext;
>
> - wl_priv_signal_emit(&display->destroy_signal, display);
> + wl_signal_emit(&display->destroy_signal, display);
>
> wl_list_for_each_safe(s, next, &display->socket_list, link) {
> wl_socket_destroy(s);
> @@ -1597,7 +1597,7 @@ WL_EXPORT void
> wl_display_add_destroy_listener(struct wl_display *display,
> struct wl_listener *listener)
> {
> - wl_priv_signal_add(&display->destroy_signal, listener);
> + wl_signal_add(&display->destroy_signal, listener);
> }
>
> /** Registers a listener for the client connection signal.
> @@ -1615,14 +1615,14 @@ WL_EXPORT void
> wl_display_add_client_created_listener(struct wl_display *display,
> struct wl_listener *listener)
> {
> - wl_priv_signal_add(&display->create_client_signal, listener);
> + wl_signal_add(&display->create_client_signal, listener);
> }
>
> WL_EXPORT struct wl_listener *
> wl_display_get_destroy_listener(struct wl_display *display,
> wl_notify_func_t notify)
> {
> - return wl_priv_signal_get(&display->destroy_signal, notify);
> + return wl_signal_get(&display->destroy_signal, notify);
> }
>
> WL_EXPORT void
> @@ -1679,7 +1679,7 @@ wl_resource_create(struct wl_client *client,
> resource->object.implementation = NULL;
>
> wl_signal_init(&resource->deprecated_destroy_signal);
> - wl_priv_signal_init(&resource->destroy_signal);
> + wl_signal_init(&resource->destroy_signal);
>
> resource->destroy = NULL;
> resource->client = client;
> @@ -1695,7 +1695,7 @@ wl_resource_create(struct wl_client *client,
> return NULL;
> }
>
> - wl_priv_signal_emit(&client->resource_created_signal, resource);
> + wl_signal_emit(&client->resource_created_signal, resource);
> return resource;
> }
>
> @@ -1883,7 +1883,7 @@ WL_EXPORT void
> wl_client_add_resource_created_listener(struct wl_client *client,
> struct wl_listener *listener)
> {
> - wl_priv_signal_add(&client->resource_created_signal, listener);
> + wl_signal_add(&client->resource_created_signal, listener);
> }
>
> struct wl_resource_iterator_context {
> @@ -1932,101 +1932,6 @@ wl_client_for_each_resource(struct wl_client *client,
> wl_map_for_each(&client->objects, resource_iterator_helper, &context);
> }
>
> -/** \cond INTERNAL */
> -
> -/** Initialize a wl_priv_signal object
> - *
> - * wl_priv_signal is a safer implementation of a signal type, with the same API
> - * as wl_signal, but kept as a private utility of libwayland-server.
> - * It is safer because listeners can be removed from within wl_priv_signal_emit()
> - * without corrupting the signal's list.
> - *
> - * Before passing a wl_priv_signal object to any other function it must be
> - * initialized by useing wl_priv_signal_init().
> - *
> - * \memberof wl_priv_signal
> - */
> -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
> - *
> - * The new listener will be called when calling wl_signal_emit(). If a listener is
> - * added to the signal while wl_signal_emit() is running it will be called from
> - * the next time wl_priv_signal_emit() is called.
> - * To remove a listener call wl_list_remove() on its link member.
> - *
> - * \memberof wl_priv_signal
> - */
> -void
> -wl_priv_signal_add(struct wl_priv_signal *signal, struct wl_listener *listener)
> -{
> - wl_list_insert(signal->listener_list.prev, &listener->link);
> -}
> -
> -/** Get a listener added to a signal
> - *
> - * Returns the listener added to the given \a signal and with the given
> - * \a notify function, or NULL if there isn't any.
> - * Calling this function from withing wl_priv_signal_emit() is safe and will
> - * return the correct value.
> - *
> - * \memberof wl_priv_signal
> - */
> -struct wl_listener *
> -wl_priv_signal_get(struct wl_priv_signal *signal, wl_notify_func_t notify)
> -{
> - struct wl_listener *l;
> -
> - 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;
> -}
> -
> -/** Emit the signal, calling all the installed listeners
> - *
> - * Iterate over all the listeners added to this \a signal and call
> - * their \a notify function pointer, passing on the given \a data.
> - * Removing or adding a listener from within wl_priv_signal_emit()
> - * is safe.
> - */
> -void
> -wl_priv_signal_emit(struct wl_priv_signal *signal, void *data)
> -{
> - struct wl_listener *l;
> - struct wl_list *pos;
> -
> - 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.
> - * 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;
> - l = wl_container_of(pos, l, link);
> -
> - wl_list_remove(pos);
> - wl_list_insert(&signal->listener_list, pos);
> -
> - l->notify(l, data);
> - }
> -}
> -
> -/** \endcond INTERNAL */
> -
> /** \cond */ /* Deprecated functions below. */
>
> uint32_t
> diff --git a/tests/newsignal-test.c b/tests/newsignal-test.c
> deleted file mode 100644
> index 47c429b..0000000
> --- a/tests/newsignal-test.c
> +++ /dev/null
> @@ -1,337 +0,0 @@
> -/*
> - * Copyright © 2013 Marek Chalupa
> - *
> - * Permission is hereby granted, free of charge, to any person obtaining
> - * a copy of this software and associated documentation files (the
> - * "Software"), to deal in the Software without restriction, including
> - * without limitation the rights to use, copy, modify, merge, publish,
> - * distribute, sublicense, and/or sell copies of the Software, and to
> - * permit persons to whom the Software is furnished to do so, subject to
> - * the following conditions:
> - *
> - * The above copyright notice and this permission notice (including the
> - * next paragraph) shall be included in all copies or substantial
> - * portions of the Software.
> - *
> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> - * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> - * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> - * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> - * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> - * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> - * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> - * SOFTWARE.
> - */
> -
> -#include <assert.h>
> -
> -#include "test-runner.h"
> -#include "wayland-private.h"
> -
> -static void
> -signal_notify(struct wl_listener *listener, void *data)
> -{
> - /* only increase counter*/
> - ++(*((int *) data));
> -}
> -
> -TEST(signal_init)
> -{
> - struct wl_priv_signal signal;
> -
> - wl_priv_signal_init(&signal);
> -
> - /* Test if listeners' list is initialized */
> - assert(&signal.listener_list == signal.listener_list.next
> - && "Maybe wl_priv_signal implementation changed?");
> - assert(signal.listener_list.next == signal.listener_list.prev
> - && "Maybe wl_priv_signal implementation changed?");
> -}
> -
> -TEST(signal_add_get)
> -{
> - struct wl_priv_signal signal;
> -
> - /* we just need different values of notify */
> - struct wl_listener l1 = {.notify = (wl_notify_func_t) 0x1};
> - struct wl_listener l2 = {.notify = (wl_notify_func_t) 0x2};
> - struct wl_listener l3 = {.notify = (wl_notify_func_t) 0x3};
> - /* one real, why not */
> - struct wl_listener l4 = {.notify = signal_notify};
> -
> - wl_priv_signal_init(&signal);
> -
> - wl_priv_signal_add(&signal, &l1);
> - wl_priv_signal_add(&signal, &l2);
> - wl_priv_signal_add(&signal, &l3);
> - wl_priv_signal_add(&signal, &l4);
> -
> - assert(wl_priv_signal_get(&signal, signal_notify) == &l4);
> - assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x3) == &l3);
> - assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x2) == &l2);
> - assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x1) == &l1);
> -
> - /* get should not be destructive */
> - assert(wl_priv_signal_get(&signal, signal_notify) == &l4);
> - assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x3) == &l3);
> - assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x2) == &l2);
> - assert(wl_priv_signal_get(&signal, (wl_notify_func_t) 0x1) == &l1);
> -}
> -
> -TEST(signal_emit_to_one_listener)
> -{
> - int count = 0;
> - int counter;
> -
> - struct wl_priv_signal signal;
> - struct wl_listener l1 = {.notify = signal_notify};
> -
> - wl_priv_signal_init(&signal);
> - wl_priv_signal_add(&signal, &l1);
> -
> - for (counter = 0; counter < 100; counter++)
> - wl_priv_signal_emit(&signal, &count);
> -
> - assert(counter == count);
> -}
> -
> -TEST(signal_emit_to_more_listeners)
> -{
> - int count = 0;
> - int counter;
> -
> - struct wl_priv_signal signal;
> - struct wl_listener l1 = {.notify = signal_notify};
> - struct wl_listener l2 = {.notify = signal_notify};
> - struct wl_listener l3 = {.notify = signal_notify};
> -
> - wl_priv_signal_init(&signal);
> - wl_priv_signal_add(&signal, &l1);
> - wl_priv_signal_add(&signal, &l2);
> - wl_priv_signal_add(&signal, &l3);
> -
> - for (counter = 0; counter < 100; counter++)
> - wl_priv_signal_emit(&signal, &count);
> -
> - assert(3 * counter == count);
> -}
> -
> -struct signal
> -{
> - struct wl_priv_signal signal;
> - struct wl_listener l1, l2, l3;
> - int count;
> - struct wl_listener *current;
> -};
> -
> -static void notify_remove(struct wl_listener *l, void *data)
> -{
> - struct signal *sig = data;
> - wl_list_remove(&sig->current->link);
> - wl_list_init(&sig->current->link);
> - sig->count++;
> -}
> -
> -#define INIT \
> - wl_priv_signal_init(&signal.signal); \
> - wl_list_init(&signal.l1.link); \
> - wl_list_init(&signal.l2.link); \
> - wl_list_init(&signal.l3.link);
> -#define CHECK_EMIT(expected) \
> - signal.count = 0; \
> - wl_priv_signal_emit(&signal.signal, &signal); \
> - assert(signal.count == expected);
> -
> -TEST(signal_remove_listener)
> -{
> - test_set_timeout(4);
> -
> - struct signal signal;
> -
> - signal.l1.notify = notify_remove;
> - signal.l2.notify = notify_remove;
> - signal.l3.notify = notify_remove;
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> -
> - signal.current = &signal.l1;
> - CHECK_EMIT(1)
> - CHECK_EMIT(0)
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> -
> - CHECK_EMIT(2)
> - CHECK_EMIT(1)
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> -
> - signal.current = &signal.l2;
> - CHECK_EMIT(1)
> - CHECK_EMIT(1)
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> - wl_priv_signal_add(&signal.signal, &signal.l3);
> -
> - signal.current = &signal.l1;
> - CHECK_EMIT(3)
> - CHECK_EMIT(2)
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> - wl_priv_signal_add(&signal.signal, &signal.l3);
> -
> - signal.current = &signal.l2;
> - CHECK_EMIT(2)
> - CHECK_EMIT(2)
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> - wl_priv_signal_add(&signal.signal, &signal.l3);
> -
> - signal.current = &signal.l3;
> - CHECK_EMIT(2)
> - CHECK_EMIT(2)
> -}
> -
> -static void notify_readd(struct wl_listener *l, void *data)
> -{
> - struct signal *signal = data;
> - if (signal->current) {
> - wl_list_remove(&signal->current->link);
> - wl_list_init(&signal->current->link);
> - wl_priv_signal_add(&signal->signal, signal->current);
> - }
> - signal->count++;
> -}
> -
> -static void notify_empty(struct wl_listener *l, void *data)
> -{
> - struct signal *signal = data;
> - signal->count++;
> -}
> -
> -TEST(signal_readd_listener)
> -{
> - /* Readding a listener is supported, that is it doesn't trigger an
> - * infinite loop or other weird things, but if in a listener you
> - * readd another listener, that will not be fired in the current
> - * signal emission. */
> -
> - test_set_timeout(4);
> -
> - struct signal signal;
> -
> - signal.l1.notify = notify_readd;
> - signal.l2.notify = notify_readd;
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> -
> - signal.current = &signal.l1;
> - CHECK_EMIT(1)
> - CHECK_EMIT(1)
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> -
> - signal.current = &signal.l2;
> - CHECK_EMIT(1)
> - signal.current = NULL;
> - CHECK_EMIT(2)
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> -
> - signal.current = &signal.l1;
> - CHECK_EMIT(1)
> - /* l2 was added before l1, so l2 is fired first, which by readding l1
> - * removes it from the current list that is being fired, so 1 is correct */
> - CHECK_EMIT(1)
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> -
> - signal.l1.notify = notify_empty;
> - signal.current = &signal.l2;
> - CHECK_EMIT(2)
> - CHECK_EMIT(2)
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> -
> - signal.l1.notify = notify_empty;
> - signal.current = &signal.l1;
> - CHECK_EMIT(2)
> - /* same as before, by readding l1 in the first emit, it now is fired
> - * after l2, so on the second emit it is not fired at all. */
> - CHECK_EMIT(1)
> -}
> -
> -static void notify_addandget(struct wl_listener *l, void *data)
> -{
> - struct signal *signal = data;
> - wl_list_remove(&signal->current->link);
> - wl_list_init(&signal->current->link);
> - wl_priv_signal_add(&signal->signal, signal->current);
> -
> - assert(wl_priv_signal_get(&signal->signal, signal->current->notify) != NULL);
> -
> - signal->count++;
> -}
> -
> -static void notify_get(struct wl_listener *l, void *data)
> -{
> - struct signal *signal = data;
> - assert(wl_priv_signal_get(&signal->signal, signal->current->notify) == signal->current);
> - signal->count++;
> -}
> -
> -TEST(signal_get_listener)
> -{
> - test_set_timeout(4);
> -
> - struct signal signal;
> -
> - signal.l1.notify = notify_addandget;
> - signal.l2.notify = notify_get;
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> -
> - signal.current = &signal.l2;
> - CHECK_EMIT(1)
> -
> - INIT
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> -
> - signal.current = &signal.l2;
> - CHECK_EMIT(1)
> -
> - INIT
> - signal.l1.notify = notify_get;
> - signal.l2.notify = notify_empty;
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> -
> - CHECK_EMIT(2)
> -
> - INIT
> - signal.l1.notify = notify_empty;
> - signal.l2.notify = notify_get;
> - wl_priv_signal_add(&signal.signal, &signal.l1);
> - wl_priv_signal_add(&signal.signal, &signal.l2);
> -
> - signal.current = &signal.l1;
> - CHECK_EMIT(2)
> -}
> diff --git a/tests/signal-test.c b/tests/signal-test.c
> index 7bbaa9f..52ed79b 100644
> --- a/tests/signal-test.c
> +++ b/tests/signal-test.c
> @@ -25,8 +25,8 @@
>
> #include <assert.h>
>
> -#include "wayland-server.h"
> #include "test-runner.h"
> +#include "wayland-private.h"
>
> static void
> signal_notify(struct wl_listener *listener, void *data)
> @@ -115,3 +115,223 @@ TEST(signal_emit_to_more_listeners)
>
> assert(3 * counter == count);
> }
> +
> +struct signal
> +{
> + struct wl_signal signal;
> + struct wl_listener l1, l2, l3;
> + int count;
> + struct wl_listener *current;
> +};
> +
> +static void notify_remove(struct wl_listener *l, void *data)
> +{
> + struct signal *sig = data;
> + wl_list_remove(&sig->current->link);
> + wl_list_init(&sig->current->link);
> + sig->count++;
> +}
> +
> +#define INIT \
> + wl_signal_init(&signal.signal); \
> + wl_list_init(&signal.l1.link); \
> + wl_list_init(&signal.l2.link); \
> + wl_list_init(&signal.l3.link);
> +#define CHECK_EMIT(expected) \
> + signal.count = 0; \
> + wl_signal_emit(&signal.signal, &signal); \
> + assert(signal.count == expected);
> +
> +TEST(signal_remove_listener)
> +{
> + test_set_timeout(4);
> +
> + struct signal signal;
> +
> + signal.l1.notify = notify_remove;
> + signal.l2.notify = notify_remove;
> + signal.l3.notify = notify_remove;
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> +
> + signal.current = &signal.l1;
> + CHECK_EMIT(1)
> + CHECK_EMIT(0)
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> + wl_signal_add(&signal.signal, &signal.l2);
> +
> + CHECK_EMIT(2)
> + CHECK_EMIT(1)
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> + wl_signal_add(&signal.signal, &signal.l2);
> +
> + signal.current = &signal.l2;
> + CHECK_EMIT(1)
> + CHECK_EMIT(1)
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> + wl_signal_add(&signal.signal, &signal.l2);
> + wl_signal_add(&signal.signal, &signal.l3);
> +
> + signal.current = &signal.l1;
> + CHECK_EMIT(3)
> + CHECK_EMIT(2)
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> + wl_signal_add(&signal.signal, &signal.l2);
> + wl_signal_add(&signal.signal, &signal.l3);
> +
> + signal.current = &signal.l2;
> + CHECK_EMIT(2)
> + CHECK_EMIT(2)
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> + wl_signal_add(&signal.signal, &signal.l2);
> + wl_signal_add(&signal.signal, &signal.l3);
> +
> + signal.current = &signal.l3;
> + CHECK_EMIT(2)
> + CHECK_EMIT(2)
> +}
> +
> +static void notify_readd(struct wl_listener *l, void *data)
> +{
> + struct signal *signal = data;
> + if (signal->current) {
> + wl_list_remove(&signal->current->link);
> + wl_list_init(&signal->current->link);
> + wl_signal_add(&signal->signal, signal->current);
> + }
> + signal->count++;
> +}
> +
> +static void notify_empty(struct wl_listener *l, void *data)
> +{
> + struct signal *signal = data;
> + signal->count++;
> +}
> +
> +TEST(signal_readd_listener)
> +{
> + /* Readding a listener is supported, that is it doesn't trigger an
> + * infinite loop or other weird things, but if in a listener you
> + * readd another listener, that will not be fired in the current
> + * signal emission. */
> +
> + test_set_timeout(4);
> +
> + struct signal signal;
> +
> + signal.l1.notify = notify_readd;
> + signal.l2.notify = notify_readd;
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> +
> + signal.current = &signal.l1;
> + CHECK_EMIT(1)
> + CHECK_EMIT(1)
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> +
> + signal.current = &signal.l2;
> + CHECK_EMIT(1)
> + signal.current = NULL;
> + CHECK_EMIT(2)
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l2);
> +
> + signal.current = &signal.l1;
> + CHECK_EMIT(1)
> + /* l2 was added before l1, so l2 is fired first, which by readding l1
> + * removes it from the current list that is being fired, so 1 is correct */
> + CHECK_EMIT(1)
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> + wl_signal_add(&signal.signal, &signal.l2);
> +
> + signal.l1.notify = notify_empty;
> + signal.current = &signal.l2;
> + CHECK_EMIT(2)
> + CHECK_EMIT(2)
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> + wl_signal_add(&signal.signal, &signal.l2);
> +
> + signal.l1.notify = notify_empty;
> + signal.current = &signal.l1;
> + CHECK_EMIT(2)
> + /* same as before, by readding l1 in the first emit, it now is fired
> + * after l2, so on the second emit it is not fired at all. */
> + CHECK_EMIT(1)
> +}
> +
> +static void notify_addandget(struct wl_listener *l, void *data)
> +{
> + struct signal *signal = data;
> + wl_list_remove(&signal->current->link);
> + wl_list_init(&signal->current->link);
> + wl_signal_add(&signal->signal, signal->current);
> +
> + assert(wl_signal_get(&signal->signal, signal->current->notify) != NULL);
> +
> + signal->count++;
> +}
> +
> +static void notify_get(struct wl_listener *l, void *data)
> +{
> + struct signal *signal = data;
> + assert(wl_signal_get(&signal->signal, signal->current->notify) == signal->current);
> + signal->count++;
> +}
> +
> +TEST(signal_get_listener)
> +{
> + test_set_timeout(4);
> +
> + struct signal signal;
> +
> + signal.l1.notify = notify_addandget;
> + signal.l2.notify = notify_get;
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l1);
> +
> + signal.current = &signal.l2;
> + CHECK_EMIT(1)
> +
> + INIT
> + wl_signal_add(&signal.signal, &signal.l2);
> +
> + signal.current = &signal.l2;
> + CHECK_EMIT(1)
> +
> + INIT
> + signal.l1.notify = notify_get;
> + signal.l2.notify = notify_empty;
> + wl_signal_add(&signal.signal, &signal.l1);
> + wl_signal_add(&signal.signal, &signal.l2);
> +
> + CHECK_EMIT(2)
> +
> + INIT
> + signal.l1.notify = notify_empty;
> + signal.l2.notify = notify_get;
> + wl_signal_add(&signal.signal, &signal.l1);
> + wl_signal_add(&signal.signal, &signal.l2);
> +
> + signal.current = &signal.l1;
> + CHECK_EMIT(2)
> +}
>
More information about the wayland-devel
mailing list