[PATCH wayland 3/5] Add API to get the list of connected clients

[Giulio Camuffo]

2016-06-17 13:51 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
> On Mon,  7 Mar 2016 18:31:33 +0100
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> ---
>>  src/wayland-server-core.h | 14 +++++++++++++
>>  src/wayland-server.c      | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 66 insertions(+)
>>
>> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
>> index a4ca350..4201b2c 100644
>> --- a/src/wayland-server-core.h
>> +++ b/src/wayland-server-core.h
>> @@ -176,6 +176,20 @@ wl_global_destroy(struct wl_global *global);
>>  struct wl_client *
>>  wl_client_create(struct wl_display *display, int fd);
>>
>> +struct wl_list *
>> +wl_display_get_client_list(struct wl_display *display);
>> +
>> +struct wl_list *
>> +wl_client_get_link(struct wl_client *client);
>> +
>> +struct wl_client *
>> +wl_client_from_link(struct wl_list *link);
>> +
>> +#define wl_client_for_each(client, list)                             \
>> +     for (client = 0, client = wl_client_from_link((list)->next);    \
>> +          wl_client_get_link(client) != (list);                      \
>> +          client = wl_client_from_link(wl_client_get_link(client)->next))
>> +
>
> Hi,
>
> ok, there is this style of iterating over opaque objects. Then there is
> the other style you add for iterating over resources. Why are they
> different? Shouldn't we use the same style for both?
>
> That would mean we use the funcptr callback style, because wl_resources
> are kept in a wl_map, not a wl_list. But should that be the exception,
> is it so much harder to use?
>
> We already have wl_resource_for_each{,_safe}() macros. One could have
> written a wl_list_for_each_resource(struct wl_list *resource_list, func
> iterator, void *user_data). I do not know why it wasn't made that way.
> Ease of use at the cost of more function calls into the library?
>
> Do you know why there is the 'client = 0'? It seems to be present
> already in wl_resource_for_each() and I can't figure out why.
>
> I suppose the style you chose here is the one you prefer the most?
>
> I'm fine with both ways, though I would hope the commit message
> explained the reason why go this way and not the other way.

Yes, i prefer this style, since it allows to easily use data from the
outer scope in the loop, while the iterator approach requires you to
store your context somewhere and reference it through a pointer. On
the other hand, this exposes API which is not needed otherwise, and
which could also allow the user to shoot in his foot by manipulating
the list. So i don't have a strong opinion...

>
>>  void
>>  wl_client_destroy(struct wl_client *client);
>>
>> diff --git a/src/wayland-server.c b/src/wayland-server.c
>> index 5099614..a5527eb 100644
>> --- a/src/wayland-server.c
>> +++ b/src/wayland-server.c
>> @@ -1512,6 +1512,58 @@ wl_display_get_additional_shm_formats(struct wl_display *display)
>>       return &display->additional_shm_formats;
>>  }
>>
>> +/** Get the list of currently connected clients
>> + *
>> + * \param display The display object
>> + *
>> + * This function returns a pointer to the list of clients currently
>> + * connected to the display. You can iterate on the list by using
>> + * the \a wl_client_for_each macro.
>> + *
>> + * \sa wl_client_for_each()
>> + * \sa wl_client_get_link()
>> + * \sa wl_client_from_link()
>> + *
>> + * \memberof wl_display
>> + */
>> +WL_EXPORT struct wl_list *
>> +wl_display_get_client_list(struct wl_display *display)
>> +{
>> +     return &display->client_list;
>> +}
>
> This should document how long the returned list head is valid. First I
> thought it would become invalid the moment the list is manipulated
> again, but it doesn't. Shouldn't we also say one must not change the
> list?
>
>> +
>> +/** Get the link by which a client is inserted in the client list
>> + *
>> + * \param client The client object
>> + *
>> + * \sa wl_client_for_each()
>> + * \sa wl_display_get_client_list()
>> + * \sa wl_client_from_link()
>> + *
>> + * \memberof wl_client
>> + */
>> +WL_EXPORT struct wl_list *
>> +wl_client_get_link(struct wl_client *client)
>> +{
>> +     return &client->link;
>> +}
>> +
>> +/** Get a wl_client by its link
>> + *
>> + * \param link The link of a wl_client
>> + *
>> + * \sa wl_client_for_each()
>> + * \sa wl_display_get_client_list()
>> + * \sa wl_client_get_link()
>> + *
>> + * \memberof wl_client
>> + */
>> +WL_EXPORT struct wl_client *
>> +wl_client_from_link(struct wl_list *link)
>> +{
>> +     return wl_container_of(link, (struct wl_client *)0, link);
>
> How about using container_of() instead?

Ah, i didn't know there was a container_of() in wayland too.

>
> wl_resource_from_link() could use the same treatment...
>
>> +}
>> +
>>  /** \cond */ /* Deprecated functions below. */
>>
>>  uint32_t
>
> Looks pretty good to me in general.
>
>
> Thanks,
> pq