[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 +0000 (GMT)
이상진 <lsj119 at samsung.com> 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<ppaalanen at gmail.com>
> 
> 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, &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
> thin_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/20151215/9a8bc55f/attachment.sig>