[PATCH wayland v2 1/5] server: Add new api for add socket for fd
Pekka Paalanen
ppaalanen at gmail.com
Wed Dec 2 00:51:21 PST 2015
Hi,
I don't really understand the underlying use case, but I can imagine
adding listening sockects to wl_display by fd being useful, so I have
no objections to this feature in general.
More below.
On Mon, 23 Nov 2015 19:59:19 -0800
Bryce Harrington <bryce at osg.samsung.com> wrote:
> From: Sangjin Lee <lsj119 at samsung.com>
>
> 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: Sangjin Lee <lsj119 at samsung.com>
> Acked-by: Sung-Jin Park <sj76.park at samsung.com>
> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
> src/wayland-os.c | 12 ++++++++
> src/wayland-os.h | 2 ++
> src/wayland-server-core.h | 6 ++++
> src/wayland-server.c | 76 ++++++++++++++++++++++++++++++-----------------
> 4 files changed, 68 insertions(+), 28 deletions(-)
>
> diff --git a/src/wayland-os.c b/src/wayland-os.c
> index 93b6f5f..31114f6 100644
> --- a/src/wayland-os.c
> +++ b/src/wayland-os.c
> @@ -27,6 +27,7 @@
>
> #include <sys/types.h>
> #include <sys/socket.h>
> +#include <sys/stat.h>
> #include <unistd.h>
> #include <fcntl.h>
> #include <errno.h>
> @@ -73,6 +74,17 @@ wl_os_socket_cloexec(int domain, int type, int protocol)
> }
>
> int
> +wl_os_socket_check_cloexec(int fd)
> +{
> + struct stat buf;
> +
> + if (fd < 0 || fstat(fd, &buf) < 0 || !S_ISSOCK(buf.st_mode))
> + return -1;
> +
> + return set_cloexec_or_close(fd);
> +}
Why is setting cloexec baked into this check?
It is fundamentally impossible to write an OS helper function that
would guarantee there is no race in setting the cloexec vs. forks,
because you already have the fd open.
That's why I think it would be better to just expose the
set_cloexec_or_close() with a namespaced name, rather than this new
function.
It seems there is nothing OS-specific in the ISSOCK check, or at least
you are not accounting for any, so it doesn't belong in wayland-os.c.
> +
> +int
> wl_os_dupfd_cloexec(int fd, long minfd)
> {
> int newfd;
> diff --git a/src/wayland-os.h b/src/wayland-os.h
> index f51efaa..e3e7bf0 100644
> --- a/src/wayland-os.h
> +++ b/src/wayland-os.h
> @@ -41,6 +41,8 @@ wl_os_epoll_create_cloexec(void);
> int
> wl_os_accept_cloexec(int sockfd, struct sockaddr *addr, socklen_t *addrlen);
>
> +int
> +wl_os_socket_check_cloexec(int fd);
>
> /*
> * The following are for wayland-os.c and the unit tests.
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index 85b4b9e..9710609 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -128,9 +128,15 @@ 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);
>
> +const char *
> +wl_display_add_socket_fd_auto(struct wl_display *display, int sock_fd);
These two API additions are strange, more of them below.
IMHO much better would be to add just one:
int
wl_display_add_socket_fd(struct wl_display *display, int sock_fd);
which will simply fail if sock_fd is not open and valid.
> +
> void
> wl_display_terminate(struct wl_display *display);
>
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 0f04f66..bac98bf 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1133,8 +1133,12 @@ _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) {
> + if (s->fd == -1) {
> + s->fd = wl_os_socket_cloexec(PF_LOCAL, SOCK_STREAM, 0);
> + if (s->fd < 0) {
> + return -1;
> + }
> + } else if (wl_os_socket_check_cloexec(s->fd) < 0) {
It would make sense to restructure _wl_display_add_socket() to take an
explicit fd argument and perhaps move the wl_os_socket_cloexec() to the
callers, but since that's minor internal details I won't fight over it.
> return -1;
> }
>
> @@ -1161,7 +1165,7 @@ _wl_display_add_socket(struct wl_display *display, struct wl_socket *s)
> }
>
> WL_EXPORT const char *
> -wl_display_add_socket_auto(struct wl_display *display)
> +wl_display_add_socket_fd_auto(struct wl_display *display, int sock_fd)
This makes a very strange public API: depending on sock_fd, this is
either "fd" or "auto" behaviour.
I'd prefer this to be a helper that the public entry points called, so
that wl_display_add_socket_fd() would ensure the given fd is valid or
fail. It should not silently fall back to creating a new socket file
just if sock_fd happens to be -1 due to an earlier error.
> {
> struct wl_socket *s;
> int displayno = 0;
> @@ -1185,6 +1189,8 @@ wl_display_add_socket_auto(struct wl_display *display)
> if (wl_socket_lock(s) < 0)
> continue;
>
> + s->fd = sock_fd;
> +
> if (_wl_display_add_socket(display, s) < 0) {
> wl_socket_destroy(s);
> return NULL;
> @@ -1199,32 +1205,14 @@ wl_display_add_socket_auto(struct wl_display *display)
> return NULL;
> }
>
> -/** Add a socket to Wayland display for the clients to connect.
> - *
> - * \param display Wayland display to which the socket should be added.
> - * \param name Name of the Unix socket.
> - * \return 0 if success. -1 if failed.
> - *
> - * This adds a Unix socket to Wayland display which can be used by clients to
> - * connect to Wayland display.
> - *
> - * If NULL is passed as name, then it would look for WAYLAND_DISPLAY env
> - * variable for the socket name. If WAYLAND_DISPLAY is not set, then default
> - * wayland-0 is used.
> - *
> - * The Unix socket will be created in the directory pointed to by environment
> - * variable XDG_RUNTIME_DIR. If XDG_RUNTIME_DIR is not set, then this function
> - * fails and returns -1.
> - *
> - * The length of socket path, i.e., the path set in XDG_RUNTIME_DIR and the
> - * socket name, must not exceed the maxium length of a Unix socket path.
> - * The function also fails if the user do not have write permission in the
> - * XDG_RUNTIME_DIR path or if the socket name is already in use.
> - *
> - * \memberof wl_display
> - */
> +WL_EXPORT const char *
> +wl_display_add_socket_auto(struct wl_display *display)
> +{
> + return wl_display_add_socket_fd_auto(display, -1);
> +}
> +
> WL_EXPORT int
> -wl_display_add_socket(struct wl_display *display, const char *name)
> +wl_display_add_socket_fd(struct wl_display *display, const char *name, int sock_fd)
The same comments here.
If you are calling wl_display_add_socket_fd(), it should fail if the fd
is not valid. The name argument that may or may not be used makes for a
confusing public API.
> {
> struct wl_socket *s;
>
> @@ -1247,6 +1235,8 @@ wl_display_add_socket(struct wl_display *display, const char *name)
> return -1;
> }
>
> + s->fd = sock_fd;
> +
> if (_wl_display_add_socket(display, s) < 0) {
> wl_socket_destroy(s);
> return -1;
> @@ -1255,6 +1245,36 @@ wl_display_add_socket(struct wl_display *display, const char *name)
> return 0;
> }
>
> +/** Add a socket to Wayland display for the clients to connect.
> + *
> + * \param display Wayland display to which the socket should be added.
> + * \param name Name of the Unix socket.
> + * \return 0 if success. -1 if failed.
> + *
> + * This adds a Unix socket to Wayland display which can be used by clients to
> + * connect to Wayland display.
> + *
> + * If NULL is passed as name, then it would look for WAYLAND_DISPLAY env
> + * variable for the socket name. If WAYLAND_DISPLAY is not set, then default
> + * wayland-0 is used.
> + *
> + * The Unix socket will be created in the directory pointed to by environment
> + * variable XDG_RUNTIME_DIR. If XDG_RUNTIME_DIR is not set, then this function
> + * fails and returns -1.
> + *
> + * The length of socket path, i.e., the path set in XDG_RUNTIME_DIR and the
> + * socket name, must not exceed the maxium length of a Unix socket path.
> + * The function also fails if the user do not have write permission in the
> + * XDG_RUNTIME_DIR path or if the socket name is already in use.
> + *
> + * \memberof wl_display
> + */
> +WL_EXPORT int
> +wl_display_add_socket(struct wl_display *display, const char *name)
> +{
> + return wl_display_add_socket_fd(display, name, -1);
> +}
> +
> WL_EXPORT void
> wl_display_add_destroy_listener(struct wl_display *display,
> struct wl_listener *listener)
I think the new public API could be better, so NAK.
Thanks,
pq
-------------- 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/20151202/3bcfb620/attachment.sig>
More information about the wayland-devel
mailing list