[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