Re: [PATCH weston v2 5/6] Add API to retrieve and iterate over the resources list of a client
On Tue, 9 Aug 2016 18:03:21 +0800
Jonas Ådahl wrote:
> On Tue, Aug 09, 2016 at 11:51:18AM +0200, Giulio Camuffo wrote:
> > 2016-08-09 10:17 GMT+02:00 Jonas Ådahl :
> > > 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
> > >> 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?
The whole wl_map*() set of API in wayland-util.c is exported for no use
AFAIK. Maybe we should indeed try to unexport it all and see if anyone
notices.
Oh, except the tests... at least wl_map_for_each is not used by the
tests, but maybe that's rather an indication that there is a test
missing.
Thanks,
pq
pgpPA_NuOgTvp.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston v2 5/6] Add API to retrieve and iterate over the resources list of a client
On Tue, Aug 09, 2016 at 11:51:18AM +0200, Giulio Camuffo wrote:
> 2016-08-09 10:17 GMT+02:00 Jonas Ådahl :
> > 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
> >
> > 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
> >
> >> ---
> >>
> >> 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,
Re: [PATCH weston v2 5/6] Add API to retrieve and iterate over the resources list of a client
2016-08-09 10:17 GMT+02:00 Jonas Ådahl :
> 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
>
> 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
>
>> ---
>>
>> 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
>> +
Re: [PATCH weston v2 5/6] Add API to retrieve and iterate over the resources list of a client
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
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.
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.
With that said, the code looks correct to me, and with a couple of
comments inline, this is:
Reviewed-by: Jonas Ådahl
> ---
>
> 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.
>
> 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,
> + vo
[PATCH weston v2 5/6] Add API to retrieve and iterate over the resources list of a client
To complement on the new resource created signal, this allows to
iterate over the existing resources of a client.
Signed-off-by: Giulio Camuffo
---
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);
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
*,
+ 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.
+ *
+ * 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
