[PATCH wayland] server: allow removing any wl_listener from a signal when emitted
Giulio Camuffo
giuliocamuffo at gmail.com
Tue Sep 6 11:45:26 UTC 2016
Hi,
Thanks for the great wrap-up!
>
>
> A proposal
>
> I believe we could have a private and safe variant of wl_signal used
> internally for all these. We would still use struct wl_listener as is,
> and be completely ABI compatible.
>
> wl_signal would be left as is. If someone needs a wl_signal
> implementation that is completely safe against the list manipulation
> during iterating, they should use their own. Libwayland API does not
> pass wl_signals around. It's not libwayland-server's job to offer these
> misc generic tools any more than it needs in its own API. We could even
> deprecate wl_signal if we like, but it's not worth the hassle to
> actually remove it.
>
> This means that we would probably copy the safe implementation to
> Weston, but I think that would be for the better. Libweston at least
> can break its ABI still.
>
> One culprit here is that the deprecated definition of wl_resource
> contains a wl_signal member. I'm not sure what to do with that.
This seems like the better approach, i'll try to work on it soon.
>
>
>
> Anyway, there is an exposed bug right now in Weston, isn't there? We
> probably need to find a temporary fix for it until we can fix
> libwayland-server. It's too late in the release cycle to fix
> libwayland-server right now, and releasing a broken Weston wouldn't be
> nice.
I'm not aware of a case when this happens now. The problem i hit is fixed by
https://cgit.freedesktop.org/wayland/weston/commit/?id=1714f01e0cfcc6b71946d5c8d9898a2eaba86fac
which causes the view to be destroyed later, bypassing the problem. It
may still happen though, in some hidden place.
Cheers,
Giulio
>
>
> Thanks,
> pq
>
> PS. Excellent to include tests! Maybe the final solution can be based
> on this patch.
>
>> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
>> index 2c215e4..a0a3ead 100644
>> --- a/src/wayland-server-core.h
>> +++ b/src/wayland-server-core.h
>> @@ -341,21 +341,8 @@ wl_signal_get(struct wl_signal *signal, wl_notify_func_t notify)
>> return NULL;
>> }
>>
>> -/** Emits this signal, notifying all registered listeners.
>> - *
>> - * \param signal The signal object that will emit the signal
>> - * \param data The data that will be emitted with the signal
>> - *
>> - * \memberof wl_signal
>> - */
>> -static inline void
>> -wl_signal_emit(struct wl_signal *signal, void *data)
>> -{
>> - struct wl_listener *l, *next;
>> -
>> - wl_list_for_each_safe(l, next, &signal->listener_list, link)
>> - l->notify(l, data);
>> -}
>> +void
>> +wl_signal_emit(struct wl_signal *signal, void *data);
>>
>> typedef void (*wl_resource_destroy_func_t)(struct wl_resource *resource);
>>
>> diff --git a/src/wayland-server.c b/src/wayland-server.c
>> index 9ecfd97..93e838a 100644
>> --- a/src/wayland-server.c
>> +++ b/src/wayland-server.c
>> @@ -1742,6 +1742,27 @@ wl_client_for_each_resource(struct wl_client *client,
>> wl_map_for_each(&client->objects, resource_iterator_helper, &context);
>> }
>>
>> +static enum wl_iterator_result
>> +notify_listener(struct wl_list *link, void *data)
>> +{
>> + struct wl_listener *l = wl_container_of(link, l, link);
>> + l->notify(l, data);
>> + return WL_ITERATOR_CONTINUE;
>> +}
>> +
>> +/** Emits this signal, notifying all registered listeners.
>> + *
>> + * \param signal The signal object that will emit the signal
>> + * \param data The data that will be emitted with the signal
>> + *
>> + * \memberof wl_signal
>> + */
>> +WL_EXPORT void
>> +wl_signal_emit(struct wl_signal *signal, void *data)
>> +{
>> + wl_list_iterate_safe(&signal->listener_list, notify_listener, data);
>> +}
>> +
>> /** \cond */ /* Deprecated functions below. */
>>
>> uint32_t
>> diff --git a/src/wayland-util.c b/src/wayland-util.c
>> index 639ccf8..c6193b7 100644
>> --- a/src/wayland-util.c
>> +++ b/src/wayland-util.c
>> @@ -93,6 +93,35 @@ wl_list_insert_list(struct wl_list *list, struct wl_list *other)
>> }
>>
>> WL_EXPORT void
>> +wl_list_iterate_safe(struct wl_list *list,
>> + wl_list_iterator_func_t it, void *data)
>> +{
>> + struct wl_list temp;
>> + struct wl_list *pos;
>> +
>> + wl_list_init(&temp);
>> +
>> + /* 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(list)) {
>> + pos = list->next;
>> +
>> + wl_list_remove(pos);
>> + wl_list_insert(&temp, pos);
>> +
>> + if (it(pos, data) == WL_ITERATOR_STOP)
>> + break;
>> + }
>> +
>> + /* Now add back all the items in the original list. */
>> + wl_list_insert_list(list, &temp);
>> +}
>> +
>> +WL_EXPORT void
>> wl_array_init(struct wl_array *array)
>> {
>> memset(array, 0, sizeof *array);
>> diff --git a/src/wayland-util.h b/src/wayland-util.h
>> index cacc122..34697b5 100644
>> --- a/src/wayland-util.h
>> +++ b/src/wayland-util.h
>> @@ -76,6 +76,17 @@ struct wl_interface {
>> const struct wl_message *events;
>> };
>>
>> +/** \enum wl_iterator_result
>> + *
>> + * This enum represents the return value of an iterator function.
>> + */
>> +enum wl_iterator_result {
>> + /** Stop the iteration */
>> + WL_ITERATOR_STOP,
>> + /** Continue the iteration */
>> + WL_ITERATOR_CONTINUE
>> +};
>> +
>> /** \class wl_list
>> *
>> * \brief doubly-linked list
>> @@ -139,6 +150,13 @@ wl_list_empty(const struct wl_list *list);
>> void
>> wl_list_insert_list(struct wl_list *list, struct wl_list *other);
>>
>> +typedef enum wl_iterator_result (*wl_list_iterator_func_t)(struct wl_list *elm,
>> + void *data);
>> +
>> +void
>> +wl_list_iterate_safe(struct wl_list *list,
>> + wl_list_iterator_func_t iterator, void *data);
>> +
>> /**
>> * Retrieves a pointer to the containing struct of a given member item.
>> *
>> @@ -311,17 +329,6 @@ typedef int (*wl_dispatcher_func_t)(const void *, void *, uint32_t,
>>
>> typedef void (*wl_log_func_t)(const char *, va_list) WL_PRINTF(1, 0);
>>
>> -/** \enum wl_iterator_result
>> - *
>> - * This enum represents the return value of an iterator function.
>> - */
>> -enum wl_iterator_result {
>> - /** Stop the iteration */
>> - WL_ITERATOR_STOP,
>> - /** Continue the iteration */
>> - WL_ITERATOR_CONTINUE
>> -};
>> -
>> #ifdef __cplusplus
>> }
>> #endif
>> diff --git a/tests/signal-test.c b/tests/signal-test.c
>> index 7bbaa9f..1cc5883 100644
>> --- a/tests/signal-test.c
>> +++ b/tests/signal-test.c
>> @@ -115,3 +115,75 @@ TEST(signal_emit_to_more_listeners)
>>
>> assert(3 * counter == count);
>> }
>> +
>> +static void notify_remove(struct wl_listener *l, void *data)
>> +{
>> + struct wl_listener *l2 = data;
>> + wl_list_remove(&l2->link);
>> + wl_list_init(&l2->link);
>> +}
>> +
>> +static void notify(struct wl_listener *l, void *data)
>> +{}
>> +
>> +#define INIT \
>> + wl_signal_init(&signal); \
>> + wl_list_init(&l1.link); \
>> + wl_list_init(&l2.link); \
>> + wl_list_init(&l3.link); \
>> +
>> +TEST(signal_remove_listener)
>> +{
>> + test_set_timeout(4);
>> +
>> + struct wl_signal signal;
>> + struct wl_listener l1, l2, l3;
>> +
>> + l1.notify = notify_remove;
>> + l2.notify = notify;
>> + l3.notify = notify;
>> +
>> + INIT
>> + wl_signal_add(&signal, &l1);
>> +
>> + wl_signal_emit(&signal, &l1);
>> + wl_signal_emit(&signal, &l1);
>> +
>> + INIT
>> + wl_signal_add(&signal, &l1);
>> + wl_signal_add(&signal, &l2);
>> +
>> + wl_signal_emit(&signal, &l1);
>> + wl_signal_emit(&signal, &l1);
>> +
>> + INIT
>> + wl_signal_add(&signal, &l1);
>> + wl_signal_add(&signal, &l2);
>> +
>> + wl_signal_emit(&signal, &l2);
>> + wl_signal_emit(&signal, &l2);
>> +
>> + INIT
>> + wl_signal_add(&signal, &l1);
>> + wl_signal_add(&signal, &l2);
>> + wl_signal_add(&signal, &l3);
>> +
>> + wl_signal_emit(&signal, &l1);
>> + wl_signal_emit(&signal, &l1);
>> +
>> + INIT
>> + wl_signal_add(&signal, &l1);
>> + wl_signal_add(&signal, &l2);
>> + wl_signal_add(&signal, &l3);
>> +
>> + wl_signal_emit(&signal, &l2);
>> + wl_signal_emit(&signal, &l2);
>> +
>> + INIT
>> + wl_signal_add(&signal, &l1);
>> + wl_signal_add(&signal, &l2);
>> + wl_signal_add(&signal, &l3);
>> +
>> + wl_signal_emit(&signal, &l3);
>> + wl_signal_emit(&signal, &l3);
>> +}
>
More information about the wayland-devel
mailing list