[PATCH v2] server: add wl_display_destroy_clients()

[Simon Ser]

>-------- Original Message --------
>Subject: Re: [PATCH v2] server: add wl_display_destroy_clients()
>Local Time: December 13, 2017 12:11 PM
>UTC Time: December 13, 2017 11:11 AM
>From: ppaalanen at gmail.com
>To: emersion <contact at emersion.fr>
>wayland-devel at lists.freedesktop.org
>
>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

Thanks for your review!

> Do you have your compositor now calling this without issues?

Yes! I've tested the new function and it works properly. Here's the patch:
https://github.com/emersion/wlroots/commit/71d344cc616021a1d1ed6e075f8c7e0d15b0deb0

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

I'll try to make another patch for this.

> Thanks,
> pq
>