On Mon, 11 Dec 2017 22:56:32 +0100 emersion <[email protected]> wrote:
> wl_display_destroy doesn't destroy clients, so client socket file > descriptors are being kept open until the compositor process exits. > > To maintain ABI compatibility, we cannot destroy clients in > wl_display_destroy. Thus, a new wl_display_destroy_clients functions > is added and should be called by compositors right before > wl_display_destroy. > > See https://patchwork.freedesktop.org/patch/128832/ > > Signed-off-by: emersion <[email protected]> Hi, thanks for this patch, this implements the idea I suggested in the review comments of the earlier proposal. Would be good to mention https://bugs.freedesktop.org/show_bug.cgi?id=99142 in the commit message. Please wrap the commit message lines. > --- > src/wayland-server-core.h | 3 +++ > src/wayland-server.c | 22 ++++++++++++++++++++++ > 2 files changed, 25 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..7f24ef1 100644 > --- a/src/wayland-server.c > +++ b/src/wayland-server.c > @@ -1264,6 +1264,28 @@ wl_display_flush_clients(struct wl_display *display) > } > } > Documentation is missing. > +WL_EXPORT void > +wl_display_destroy_clients(struct wl_display *display) > +{ > + struct wl_list tmp_client_list; > + struct wl_client *client, *next; > + > + // Move the whole client list to a temporary head because some new > clients > + // might be added to the original head Use old style C comments /* */ please. > + wl_list_init(&tmp_client_list); > + wl_list_insert_list(&tmp_client_list, &display->client_list); > + wl_list_init(&display->client_list); Very good. > + > + wl_list_for_each_safe(client, next, &tmp_client_list, link) { > + wl_client_destroy(client); > + } I wonder if we should be even more paranoid here. This loop is not safe if the callbacks from destroying this wl_client already destroy the next wl_client. What would be guaranteed safe here is to loop while the temporary list is not empty, and always taking the first entry. An example is in wl_priv_signal_emit(). > + > + 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"); > + } Good. > +} > + > static int > socket_data(int fd, uint32_t mask, void *data) > { Thanks, pq
pgpF9_Cyrq53X.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
