[PATCH wayland 5/5] Add API to retrieve and iterate over the resources list of a client

[Giulio Camuffo]

2016-06-17 14:07 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
> On Mon,  7 Mar 2016 18:31:35 +0100
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> To complement on the new resource created signal, this allows to
>> iterate over the existing resources of a client.
>> ---
>>  src/wayland-server-core.h |  8 ++++++++
>>  src/wayland-server.c      | 23 +++++++++++++++++++++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
>> index 9af2481..9980c29 100644
>> --- a/src/wayland-server-core.h
>> +++ b/src/wayland-server-core.h
>> @@ -221,6 +221,14 @@ void
>>  wl_client_add_resource_created_listener(struct wl_client *client,
>>                                          struct wl_listener *listener);
>>
>> +typedef void (*wl_client_for_each_resource_iterator_func_t)(struct wl_resource *,
>> +                                                            void *user_data);
>> +
>> +void
>> +wl_client_for_each_resource(struct wl_client *client,
>> +                            wl_client_for_each_resource_iterator_func_t iterator,
>> +                            void *user_data);
>
> Hi,
>
> I wrote some pondering for patch 3 about the style of the API. Not much
> choice here because of the wl_map.
>
> Would you ever want to stop iterating in the middle? Should we return a
> bool or int from the func_t to signal early return?

Right, that's a good idea.

>
>> +
>>  /** \class wl_listener
>>   *
>>   * \brief A single listener for Wayland signals
>> diff --git a/src/wayland-server.c b/src/wayland-server.c
>> index 56e17c3..e47ccec 100644
>> --- a/src/wayland-server.c
>> +++ b/src/wayland-server.c
>> @@ -1584,6 +1584,29 @@ wl_client_add_resource_created_listener(struct wl_client *client,
>>       wl_signal_add(&client->resource_created_signal, listener);
>>  }
>>
>> +/** Iterate over all the resources of a client
>> + *
>> + * \param client The client object
>> + * \param iterator The iterator function
>> + * \param user_data The user data pointer
>> + *
>> + * The function pointed by \a iterator will be called for each
>> + * resource owned by the client. The \a user_data will be passed
>> + * as the second argument of the iterator function.
>> + *
>> + * \memberof wl_client
>
> Is it safe to create or destroy wl_resources while iterating, or is it
> forbidden?
>
> Please, document that.

Will do.

>
>> + */
>> +WL_EXPORT void
>> +wl_client_for_each_resource(struct wl_client *client,
>> +                         wl_client_for_each_resource_iterator_func_t iterator,
>> +                         void *user_data)
>> +{
>> +     /* wl_iterator_func_t passes wl_object as the first argument, and
>> +      * wl_resource is a wl_object, so we can safely cast it. */
>> +     wl_iterator_func_t it = (wl_iterator_func_t)iterator;
>
> This function-pointer-casting is a little odd, since it will never be
> cast back to the correct one before calling it. I'm sure it works as
> is, but... this could really use a test in 'make check', if not wrapped
> in a correct type.

To avoid the cast we'd need to use a dummy iterator calling the actual
one, resulting in one more indirection and one malloc to store the
user data and the iterator. Maybe it's not a big deal after all, but
i'd prefer to avoid it. So i'll add a test.


Thanks,
Giulio

>
>> +     wl_map_for_each(&client->objects, it, user_data);
>> +}
>> +
>>  /** \cond */ /* Deprecated functions below. */
>>
>>  uint32_t
>
> Looks good otherwise.
>
>
> Thanks,
> pq