[PATCH weston v2 5/6] Add API to retrieve and iterate over the resources list of a client
Jonas Ådahl
jadahl at gmail.com
Tue Aug 9 10:03:21 UTC 2016
On Tue, Aug 09, 2016 at 11:51:18AM +0200, Giulio Camuffo wrote:
> 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.
I'd say returning an enum is better than returning an int. Passing
around a bool pointer is fine too i guess. Both are easier to read than
just a special int return value.
>
>
> >
> > 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?
I suspect it's from when things were moving between wayland and
weston/wayland-demos. Could we maybe even unexport it? Would anything
break?
Jonas
>
>
> 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