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

[이상진]

Title: Samsung Enterprise Portal mySingle


This patch set did not consider the socket activation.So the fd is the unbind, no listen fd. The name was needed for the bind().However socket activation seems to be good point.If i am considering a socket activation, as you said wl_display_add_socket_fd () is only wl_event_loop_add_fd () and wl_list_insert () just needed.
 
--- Original Message ---
Sender : Pekka Paalanen
Date : 2015-12-14 19:42 (GMT+09:00)
Title : Re: [PATCH wayland v3 6/7] server: Add new API for adding a socket with an existing fd
 On Mon,  7 Dec 2015 22:49:18 -0800Bryce Harrington 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 > Cc: Sung-Jin Park > Cc: Sangjin Lee > --->  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 whatthis function should actually do.The use case you describe sounds very similar to socket activation. Wecan 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, ) < 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. Youcannot 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 socketwill be created in $XDG_RUNTIME_DIR/$name which I think in your usecase 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 beencreated and opened. If the process that provided you the open socket fddid locking properly, then this call would fail always. But, it mostlikely 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  compositorsWhatever 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 isproperly CLOEXEC. This can be done purely by documentation.In this function, we have no guarantees what is going on in the processwith respect to threads and forking, so there is no way we canguarantee that setting CLOEXEC cannot race.If we push the responsibility of CLOEXEC to the caller, the caller caneither provide the guarantees itself or require its caller to providethem. The caller can use recvmsg(MSG_CMSG_CLOEXEC) for instance toguarantee that the fd has CLOEXEC already when it appears in thisprocess.> + 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 andthe guessed, likely incorrect socket name.I think bind() is what will actually create the socket file in the filesystem. I'm not 

Re: [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  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 
> Cc: Sung-Jin Park 
> Cc: Sangjin Lee 
> ---
>  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, ) < 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 

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

[Pekka Paalanen]

On Tue, 15 Dec 2015 01:16:37 + (GMT)
이상진  wrote:

> Samsung Enterprise Portal mySingle
> 
> This patch set did not consider the socket activation.
> So the fd is the unbind, no listen fd. The name was needed for the bind().

Hi,

even if we did not want to support socket activation, the fd cannot be
unbound, because binding is what creates the socket file. Passing in an
unbound socket fd seems completely useless to me, because then there
is no benefit from creating the socket fd outside of libwayland-server,
and that would defeat the whole purpose of the
wl_display_add_socket_fd() API.

> However socket activation seems to be good point.
> If i am considering a socket activation, as you said
> wl_display_add_socket_fd () is only wl_event_loop_add_fd () and
> wl_list_insert () just needed.

Aiming to support socket activation forces the design of
wl_display_add_socket_fd() to be useful for the widest range of use
cases.

I am happy you will consider that.


Thanks,
pq

> 
>  
> 
> --- Original Message ---
> 
> Sender : Pekka Paalanen
> 
> Date : 2015-12-14 19:42 (GMT+09:00)
> 
> Title : Re: [PATCH wayland v3 6/7] server: Add new API for adding a
> socket with an existing fd
> 
>  On Mon,  7 Dec 2015 22:49:18 -0800
> Bryce Harrington 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
> > Cc: Sung-Jin Park
> > Cc: Sangjin Lee
> > ---
> >  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, ) < 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