[PATCH weston v2 5/6] Add API to retrieve and iterate over the resources list of a client
Giulio Camuffo
giuliocamuffo at gmail.com
Tue Aug 9 09:51:18 UTC 2016
2016-08-09 10:17 GMT+02:00 Jonas Ådahl <jadahl at gmail.com>:
> On Tue, Jul 05, 2016 at 09:51:10AM +0200, Giulio Camuffo wrote:
>> To complement on the new resource created signal, this allows to
>> iterate over the existing resources of a client.
>>
>> Signed-off-by: Giulio Camuffo <giulio.camuffo at kdab.com>
>
> I know the break:ability of the iteration was discussed in the previous
> version. I can't think of a situation where it'll ever break. If you
> need to iterate through all the resources to get a particular one, you
> are probably doing something wrong. It also makes the API a bit more
> confusing, since a numeric return value that effects a for-each loop
> will always be hard to follow.
Say that a tool has a list view to show the resources, it may want to
break the loop when the visible part of the list is full.
>
> FWIW, glib tries to solve a similiar API issue by trying to make people
> return G_SOURCE_CONTINUE and G_SOURCE_REMOVE for a similar situation,
> because returning FALSE/TRUE made reading the code annoying, but without
> actually using a enum or something like that forcing people to write
> more readable code, it's almost pointless.
I'm not sure i understand. Are you saying using an enum would be a
good or bad idea?
An alternative solution could be to keep a void return type, and to
pass a "bool *continue" to the iterator, initially
initialized to true, which the iterator sets to false when it wants to
break the loop. So in the easy case the iterator doesn't
need to care about it.
>
> With that said, the code looks correct to me, and with a couple of
> comments inline, this is:
>
> Reviewed-by: Jonas Ådahl <jadahl at gmail.com>
>
>> ---
>>
>> v2: - don't cast the function pointer, but make it a two step process
>> - make it possible to break the loop early
>>
>> src/wayland-private.h | 2 +-
>> src/wayland-server-core.h | 8 ++++++++
>> src/wayland-server.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>> src/wayland-util.c | 8 ++++++--
>> 4 files changed, 61 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/wayland-private.h b/src/wayland-private.h
>> index 045109b..b715534 100644
>> --- a/src/wayland-private.h
>> +++ b/src/wayland-private.h
>> @@ -74,7 +74,7 @@ struct wl_map {
>> uint32_t free_list;
>> };
>>
>> -typedef void (*wl_iterator_func_t)(void *element, void *data);
>> +typedef int (*wl_iterator_func_t)(void *element, void *data);
>
> Not sure if this matters, but this changes the type signature of an
> exported function symbol: wl_map_for_each. While WL_EXPORT:ed, it's not
> exposed via any installed header file, so I suppose it won't cause any
> real problems.
Oh, indeed... i wonder why it's exported. But i don't think we care,
our API is defined by the headers, right?
Thanks,
Giulio
>
>>
>> void
>> wl_map_init(struct wl_map *map, uint32_t side);
>> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
>> index bb0a989..647d97c 100644
>> --- a/src/wayland-server-core.h
>> +++ b/src/wayland-server-core.h
>> @@ -216,6 +216,14 @@ void
>> wl_client_add_resource_created_listener(struct wl_client *client,
>> struct wl_listener *listener);
>>
>> +typedef int (*wl_client_for_each_resource_iterator_func_t)(struct wl_resource *,
>
> nit: missing parameter name
>
>> + 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);
>> +
>> /** \class wl_listener
>> *
>> * \brief A single listener for Wayland signals
>> diff --git a/src/wayland-server.c b/src/wayland-server.c
>> index 067d8a5..98e3bda 100644
>> --- a/src/wayland-server.c
>> +++ b/src/wayland-server.c
>> @@ -565,7 +565,7 @@ wl_resource_post_no_memory(struct wl_resource *resource)
>> WL_DISPLAY_ERROR_NO_MEMORY, "no memory");
>> }
>>
>> -static void
>> +static int
>> destroy_resource(void *element, void *data)
>> {
>> struct wl_resource *resource = element;
>> @@ -580,6 +580,8 @@ destroy_resource(void *element, void *data)
>>
>> if (!(flags & WL_MAP_ENTRY_LEGACY))
>> free(resource);
>> +
>> + return 0;
>> }
>>
>> WL_EXPORT void
>> @@ -1603,6 +1605,49 @@ wl_client_add_resource_created_listener(struct wl_client *client,
>> wl_signal_add(&client->resource_created_signal, listener);
>> }
>>
>> +struct wl_resource_iterator_context {
>> + void *user_data;
>> + wl_client_for_each_resource_iterator_func_t it;
>> +};
>> +
>> +static int
>> +resource_iterator_helper(void *res, void *user_data)
>> +{
>> + struct wl_resource_iterator_context *context = user_data;
>> + struct wl_resource *resource = res;
>> + return context->it(resource, context->user_data);
>> +}
>> +
>> +/** 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.
>> + * If the \a iterator function returns a value less than 0 the loop
>> + * will stop.
>
> nit: "... the iteration will stop."
>
>
> Jonas
>
>> + *
>> + * Creating and destroying resources while iterating is safe, but new
>> + * resources may or may not be picked up by the iterator.
>> + *
>> + * \memberof wl_client
>> + */
>> +WL_EXPORT void
>> +wl_client_for_each_resource(struct wl_client *client,
>> + wl_client_for_each_resource_iterator_func_t iterator,
>> + void *user_data)
>> +{
>> + struct wl_resource_iterator_context context = {
>> + .user_data = user_data,
>> + .it = iterator,
>> + };
>> +
>> + wl_map_for_each(&client->objects, resource_iterator_helper, &context);
>> +}
>> +
>> /** \cond */ /* Deprecated functions below. */
>>
>> uint32_t
>> diff --git a/src/wayland-util.c b/src/wayland-util.c
>> index 7467366..856e53b 100644
>> --- a/src/wayland-util.c
>> +++ b/src/wayland-util.c
>> @@ -363,13 +363,17 @@ static void
>> for_each_helper(struct wl_array *entries, wl_iterator_func_t func, void *data)
>> {
>> union map_entry *start, *end, *p;
>> + int ret;
>>
>> start = entries->data;
>> end = (union map_entry *) ((char *) entries->data + entries->size);
>>
>> for (p = start; p < end; p++)
>> - if (p->data && !map_entry_is_free(*p))
>> - func(map_entry_get_data(*p), data);
>> + if (p->data && !map_entry_is_free(*p)) {
>> + ret = func(map_entry_get_data(*p), data);
>> + if (ret < 0)
>> + break;
>> + }
>> }
>>
>> WL_EXPORT void
>> --
>> 2.9.0
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list