[PATCH wayland 6/6] server: Add a simple API to find a good default display

[Pekka Paalanen]

On Fri, 18 Jul 2014 11:05:03 +0200
Marek Chalupa <mchqwerty at gmail.com> wrote:

> On 17 July 2014 19:54, Jasper St. Pierre <jstpierre at mecheye.net> wrote:
> 
> > This allows compositors to easily select a good display to listen on.
> > ---
> >  src/wayland-server.c | 97
> > ++++++++++++++++++++++++++++++++++++++--------------
> >  src/wayland-server.h |  1 +
> >  2 files changed, 73 insertions(+), 25 deletions(-)

Hi,

the final series that actually got pushed was never on the mailing
list, so sorry for the crappy quote.

I have one observation.


   server: Add a simple API to find a good default display
    
    This allows compositors to easily select a good display to listen on.

----------------------------- src/wayland-server.c -----------------------------
index b3929ed..39d29ec 100644
@@ -1093,19 +1093,90 @@ wl_socket_init_for_display_name(struct wl_socket *s, const char *name)
 		errno = ENAMETOOLONG;
 		return -1;
 	};
 
 	return 0;
 }
 
+static int
+_wl_display_add_socket(struct wl_display *display, struct wl_socket *s)
+{
+	socklen_t size;
+
+	s->fd = wl_os_socket_cloexec(PF_LOCAL, SOCK_STREAM, 0);
+	if (s->fd < 0) {
+		return -1;
+	}
+
+	size = offsetof (struct sockaddr_un, sun_path) + strlen(s->addr.sun_path);
+	if (bind(s->fd, (struct sockaddr *) &s->addr, size) < 0) {
+		wl_log("bind() failed with error: %m\n");
+		return -1;
+	}
+
+	if (listen(s->fd, 1) < 0) {
+		wl_log("listen() failed with error: %m\n");
+		return -1;
+	}
+
+	s->source = wl_event_loop_add_fd(display->loop, s->fd,
+					 WL_EVENT_READABLE,
+					 socket_data, display);
+	if (s->source == NULL) {
+		return -1;
+	}
+
+	wl_list_insert(display->socket_list.prev, &s->link);
+	return 0;
+}
+
+WL_EXPORT const char *
+wl_display_add_socket_auto(struct wl_display *display)
+{
+	struct wl_socket *s;
+	int displayno = 0;
+	char display_name[16] = "";
+
+	/* A reasonable number of maximum default sockets. If
+	 * you need more than this, use the explicit add_socket API. */
+	const int MAX_DISPLAYNO = 32;
+
+	s = malloc(sizeof *s);
+	memset(s, 0, sizeof *s);
+	if (s == NULL)
+		return NULL;
+
+	do {
+		snprintf(display_name, sizeof display_name, "wayland-%d", displayno);
+		if (wl_socket_init_for_display_name(s, display_name) < 0) {
+			wl_socket_destroy(s);
+			return NULL;
+		}
+
+		if (wl_socket_lock(s) < 0)
+			continue;
+
+		if (_wl_display_add_socket(display, s) < 0) {
+			wl_socket_destroy(s);
+			return NULL;
+		}

Let's imagine:

First iteration: socket already used => wl_socket_lock() fails.
Second iteration: wl_socket_init_for_display_name() happens to fail.
=> wl_socket_destroy() unlinks the existing socket's lock file.

The trick here is that reading what wl_socket_init_for_display_name()
does, it probably cannot fail on the second iteration. Would be nice to
have at least a comment here to that effect, otherwise some change in
the future may break that, as to me it seems fragile.

Or better, wl_socket_lock() should reset the lock file name before
returning an error.

+
+		return s->display_name;
+	} while (displayno++ < MAX_DISPLAYNO);
+
+	/* Ran out of display names. */
+	wl_socket_destroy(s);
+	errno = EINVAL;
+	return NULL;
+}
+
 WL_EXPORT int
 wl_display_add_socket(struct wl_display *display, const char *name)
 {
 	struct wl_socket *s;
-	socklen_t size;
 
 	s = malloc(sizeof *s);
 	memset(s, 0, sizeof *s);
 	if (s == NULL)
 		return -1;
 
 	if (name == NULL)
@@ -1119,42 +1190,19 @@ wl_display_add_socket(struct wl_display *display, const char *name)
 	}
 
 	if (wl_socket_lock(s) < 0) {
 		wl_socket_destroy(s);


Hmm, I think there is a similar problem here. If lock fails, the
existing lock file will be unlinked from under the existing compositor.


Thanks,
pq

 		return -1;
 	}
 
-	s->fd = wl_os_socket_cloexec(PF_LOCAL, SOCK_STREAM, 0);
-	if (s->fd < 0) {
+	if (_wl_display_add_socket(display, s) < 0) {
 		wl_socket_destroy(s);
 		return -1;
 	}
 
-	size = offsetof (struct sockaddr_un, sun_path) + strlen(s->addr.sun_path);
-	if (bind(s->fd, (struct sockaddr *) &s->addr, size) < 0) {
-		wl_log("bind() failed with error: %m\n");
-		wl_socket_destroy(s);
-		return -1;
-	}
-
-	if (listen(s->fd, 1) < 0) {
-		wl_log("listen() failed with error: %m\n");
-		wl_socket_destroy(s);
-		return -1;
-	}
-
-	s->source = wl_event_loop_add_fd(display->loop, s->fd,
-					 WL_EVENT_READABLE,
-					 socket_data, display);
-	if (s->source == NULL) {
-		wl_socket_destroy(s);
-		return -1;
-	}
-	wl_list_insert(display->socket_list.prev, &s->link);
-
 	return 0;
 }
 
 WL_EXPORT void
 wl_display_add_destroy_listener(struct wl_display *display,
 				struct wl_listener *listener)
 {

----------------------------- src/wayland-server.h -----------------------------
index 0a3f2f6..38855c9 100644
@@ -88,14 +88,15 @@ struct wl_listener *wl_event_loop_get_destroy_listener(
 					struct wl_event_loop *loop,
 					wl_notify_func_t notify);
 
 struct wl_display *wl_display_create(void);
 void wl_display_destroy(struct wl_display *display);
 struct wl_event_loop *wl_display_get_event_loop(struct wl_display *display);
 int wl_display_add_socket(struct wl_display *display, const char *name);
+const char *wl_display_add_socket_auto(struct wl_display *display);
 void wl_display_terminate(struct wl_display *display);
 void wl_display_run(struct wl_display *display);
 void wl_display_flush_clients(struct wl_display *display);
 
 typedef void (*wl_global_bind_func_t)(struct wl_client *client, void *data,
 				      uint32_t version, uint32_t id);