[PATCH wayland v3 6/7] server: Add new API for adding a socket with an existing fd

[Pekka Paalanen]

On Mon,  7 Dec 2015 22:49:18 -0800
Bryce Harrington <bryce at osg.samsung.com> wrote:

> Currently the server can add a socket by name.  To support an embedded
> compositor in a Simplified Mandatory Access Control Kernel (Smack)
> enabled environment, the embedded compositor should use the socket that
> it gets from the system or session compositor.
> 
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> Cc: Sung-Jin Park <sj76.park at samsung.com>
> Cc: Sangjin Lee <lsj119 at samsung.com>
> ---
>  src/wayland-server-core.h |  3 +++
>  src/wayland-server.c      | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index 85b4b9e..530053b 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -128,6 +128,9 @@ wl_display_get_event_loop(struct wl_display *display);
>  int
>  wl_display_add_socket(struct wl_display *display, const char *name);
>  
> +int
> +wl_display_add_socket_fd(struct wl_display *display, const char *name, int sock_fd);
> +
>  const char *
>  wl_display_add_socket_auto(struct wl_display *display);
>  

Hi,

this is looking better, but there are still issues with 'name' and what
this function should actually do.

The use case you describe sounds very similar to socket activation. We
can hit both with the same nice and clean API addition.

> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 7c25858..baea832 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1201,6 +1201,52 @@ wl_display_add_socket_auto(struct wl_display *display)
>  	return NULL;
>  }
>  

This new function is missing documentation.

> +WL_EXPORT int
> +wl_display_add_socket_fd(struct wl_display *display, const char *name, int sock_fd)
> +{

Why 'name' when all you have is an fd to an already open socket?

> +	struct wl_socket *s;
> +	struct stat buf;
> +
> +	/* Require a valid fd or fail */
> +	if (sock_fd < 0 || fstat(sock_fd, &buf) < 0 || !S_ISSOCK(buf.st_mode)) {
> +		return -1;
> +	}
> +
> +	s = wl_socket_alloc();
> +	if (s == NULL)
> +		return -1;
> +
> +	if (name == NULL)
> +		name = getenv("WAYLAND_DISPLAY");
> +	if (name == NULL)
> +		name = "wayland-0";

If you don't know the name, guessing it like this is harmful. You
cannot know you guessed right, because the socket is *already* open.

> +
> +	if (wl_socket_init_for_display_name(s, name) < 0) {

You cannot call this function, the socket is already open.

wl_socket_init_for_display_name() makes the assumption that the socket
will be created in $XDG_RUNTIME_DIR/$name which I think in your use
case will never be true.

> +		wl_socket_destroy(s);
> +		return -1;
> +	}
> +
> +	if (wl_socket_lock(s) < 0) {

You cannot call wl_socket_lock(), because the socket has already been
created and opened. If the process that provided you the open socket fd
did locking properly, then this call would fail always. But, it most
likely does not fail because:

- whatever creates the socket file descriptor for you is not locking
  properly, or does not need this kind of lock file

- you do not know the lock file name; you guess something in the above,
  but that is most likely wrong; it will conflict with the normal
  socket creation code, possibly causing failures elsewhere or in other
  compositors

Whatever originally creates the socket file must take care of locking.
It cannot be done here.

> +		wl_socket_destroy(s);
> +		return -1;
> +	}
> +
> +	/* Reuse the existing fd */
> +	s->fd = sock_fd;
> +	if (wl_os_set_cloexec_or_close(s->fd) < 0) {

I think we should require that the caller already ensured the fd is
properly CLOEXEC. This can be done purely by documentation.

In this function, we have no guarantees what is going on in the process
with respect to threads and forking, so there is no way we can
guarantee that setting CLOEXEC cannot race.

If we push the responsibility of CLOEXEC to the caller, the caller can
either provide the guarantees itself or require its caller to provide
them. The caller can use recvmsg(MSG_CMSG_CLOEXEC) for instance to
guarantee that the fd has CLOEXEC already when it appears in this
process.

> +		wl_log("could not set cloexec on provided fd\n");
> +		wl_socket_destroy(s);
> +		return -1;
> +	}
> +
> +	if (_wl_display_bind_socket_source(display, s) < 0) {

_wl_display_bind_socket_source() calls bind() on the given sock_fd and
the guessed, likely incorrect socket name.

I think bind() is what will actually create the socket file in the file
system. I'm not sure I understood your use case right, but it seems you
want to create the socket file outside of libwayland-server. Therefore
calling bind() seems wrong: either it should fail with EINVAL or it
creates the socket contrary to your expectations. That's my
understanding from the bind(2) manual. In any case, calling bind()
twice seems like it would be a bug.

> +		wl_socket_destroy(s);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +

Right, I think the lack of documentation of what this new function
does, assumes and requires shows up in the code as confusion about what
operations should be done here and what are the caller's
responsibilities. These need to be clarified.

You state your goal to be "Allow passing fd when adding socket for
display", for "handing out file descriptors for sockets". To me this
implies, that the API should be the following:

WL_EXPORT int
wl_display_add_socket_fd(struct wl_display *display, int sock_fd)

Where sock_fd is a file descriptor for an open socket already bound to
a socket file. If we are thinking about systemd, the sock_fd would have
already had listen() called on it too, so that socket activation worked
[1].

We do not want any systemd dependencies in libwayland, but supporting
socket activation would obviously be a good thing. So,
wl_display_add_socket_fd() should be designed to support that use case
too. It's not hard: just assume that bind() and listen() have already
been called, and don't call them again.

So what are we left with in wl_display_add_socket_fd()? Just
wl_event_loop_add_fd() and wl_list_insert(), it seems. Everything else
should be done by the caller.


Thanks,
pq

[1] http://0pointer.de/blog/projects/socket-activation.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20151214/534d38b5/attachment.sig>