Re: [PATCH wayland v2 1/5] server: Add new api for add socket for fd

[Pekka Paalanen]

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  wrote:

> From: Sangjin Lee 
> 
> 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 
> Acked-by: Sung-Jin Park 
> Reviewed-by: Bryce Harrington 
> Signed-off-by: Bryce Harrington 
> ---
>  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 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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, ) < 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 

Re: [PATCH wayland v2 1/5] server: Add new api for add socket for fd

[Pekka Paalanen]

On Wed, 2 Dec 2015 10:51:21 +0200
Pekka Paalanen  wrote:

> 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  wrote:
> 
> > From: Sangjin Lee 
> > 
> > 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 
> > Acked-by: Sung-Jin Park 
> > Reviewed-by: Bryce Harrington 
> > Signed-off-by: Bryce Harrington 
> > ---
> >  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(-)
> > 

> > --- 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.
> 

> 
> I think the new public API could be better, so NAK.

To be clear, if you fix the public API and the wayland-os part, this
would have good chances of getting accepted.

The API as proposed is a no-go for me without thorough explanation of
why you would need it the way you wrote it.


Thanks,
pq


pgpMI5ugYKvXw.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH wayland v2 1/5] server: Add new api for add socket for fd

[Bryce Harrington]

From: Sangjin Lee 

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 
Acked-by: Sung-Jin Park 
Reviewed-by: Bryce Harrington 
Signed-off-by: Bryce Harrington 
---
 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 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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, ) < 0 || !S_ISSOCK(buf.st_mode))
+   return -1;
+
+   return set_cloexec_or_close(fd);
+}
+
+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);
+
 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) {
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)
 {
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)