[PATCH v2] server: add wl_display_destroy_clients()

[Pekka Paalanen]

On Wed, 13 Dec 2017 11:51:19 +0100
emersion <contact at emersion.fr> wrote:

> Bug [1] reported that wl_display_destroy() doesn't destroy clients, so
> client socket file descriptors are being kept open until the compositor
> process exits.
> 
> Patch [2] proposed to destroy clients in wl_display_destroy(). The
> patch was not accepted because doing so changes the ABI.
> 
> Thus, a new wl_display_destroy_clients() function is added in this
> patch. It should be called by compositors right before
> wl_display_destroy().
> 
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=99142
> [2] https://patchwork.freedesktop.org/patch/128832/
> 
> Signed-off-by: emersion <contact at emersion.fr>
> ---
> v2: change commit message, add docs, use a safer approach when iterating
> over the client list
> 
>  src/wayland-server-core.h |  3 +++
>  src/wayland-server.c      | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index fd458c5..2e725d9 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -214,6 +214,9 @@ wl_display_run(struct wl_display *display);
>  void
>  wl_display_flush_clients(struct wl_display *display);
> 
> +void
> +wl_display_destroy_clients(struct wl_display *display);
> +
>  struct wl_client;
> 
>  typedef void (*wl_global_bind_func_t)(struct wl_client *client, void *data,
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 82a3b01..33df413 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1264,6 +1264,44 @@ wl_display_flush_clients(struct wl_display *display)
>  	}
>  }
> 
> +/** Destroy all clients connected to the display
> + *
> + * \param display The display object
> + *
> + * This function should be called right before wl_display_destroy() to ensure
> + * all client resources are closed properly. Destroying a client from within
> + * wl_display_destroy_clients() is safe, but creating one will leak resources
> + * and raise a warning.
> + *
> + * \memberof wl_display
> + */
> +WL_EXPORT void
> +wl_display_destroy_clients(struct wl_display *display)
> +{
> +	struct wl_list tmp_client_list, *pos;
> +	struct wl_client *client;
> +
> +	/* Move the whole client list to a temporary head because some new clients
> +	 * might be added to the original head. */
> +	wl_list_init(&tmp_client_list);
> +	wl_list_insert_list(&tmp_client_list, &display->client_list);
> +	wl_list_init(&display->client_list);
> +
> +	/* wl_list_for_each_safe isn't enough here: it fails if the next client is
> +	 * destroyed by the destroy handler of the current one. */
> +	while (!wl_list_empty(&tmp_client_list)) {
> +		pos = tmp_client_list.next;
> +		client = wl_container_of(pos, client, link);
> +
> +		wl_client_destroy(client);
> +	}
> +
> +	if (!wl_list_empty(&display->client_list)) {
> +		wl_log("wl_display_destroy_clients: cannot destroy all clients because "
> +			   "new ones were created by destroy callbacks\n");
> +	}
> +}
> +
>  static int
>  socket_data(int fd, uint32_t mask, void *data)
>  {

Hi,

very good!

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

Do you have your compositor now calling this without issues?

It would be awesome to have a test in the libwayland test suite calling
this function and checking a client gets destroyed, especially as we
don't have Weston using this yet.


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