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

[Pekka Paalanen]

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.

>  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?

wl_resource_from_link() could use the same treatment...

> +}
> +
>  /** \cond */ /* Deprecated functions below. */
>  
>  uint32_t

Looks pretty good to me in general.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160617/dce84459/attachment.sig>