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); >> +} > _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
