>-------- 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: [email protected] >To: emersion <[email protected]> >[email protected] > >On Wed, 13 Dec 2017 11:51:19 +0100 > emersion [email protected] 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 [email protected] >> >>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 [email protected]
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 > _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
