<HTML><HEAD><TITLE>Samsung Enterprise Portal mySingle</TITLE> <META content="text/html; charset=euc-kr" http-equiv=Content-Type> <STYLE id=mysingle_style type=text/css>P { MARGIN-BOTTOM: 5px; FONT-SIZE: 9pt; FONT-FAMILY: 굴림체, arial; MARGIN-TOP: 5px } TD { MARGIN-BOTTOM: 5px; FONT-SIZE: 9pt; FONT-FAMILY: 굴림체, arial; MARGIN-TOP: 5px } LI { MARGIN-BOTTOM: 5px; FONT-SIZE: 9pt; FONT-FAMILY: 굴림체, arial; MARGIN-TOP: 5px } BODY { FONT-SIZE: 9pt; FONT-FAMILY: 굴림체, arial; MARGIN: 10px; LINE-HEIGHT: 1.4 } </STYLE> <META content=IE=5 http-equiv=X-UA-Compatible> <META content=IE=5 http-equiv=X-UA-Compatible> <META name=GENERATOR content="MSHTML 11.00.9600.18098"></HEAD> <BODY> 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<ppaalanen@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 <BRYCE@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@OSG.SAMSUNG.COM> > Cc: Sung-Jin Park <SJ76.PARK@SAMSUNG.COM> > Cc: Sangjin Lee <LSJ119@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 <TABLE id=confidentialsignimg> <TBODY> <TR> <TD NAMO_LOCK> <IMG border=0 src="cid:Z5JE7EUABGFC@namo.co.kr"></TD></TR></TBODY></TABLE></BODY></HTML><img src='http://ext.samsung.net/mailcheck/SeenTimeChecker?do=b6ee01ef39bdd5c06ffc1a014d0d5390419240d3d74469d2d261643f4e17952232c2d5b092db2212f255f74e40bd48632fd4ff3ea410d012fea75954dce1604da728c55b39cc59eacf878f9a26ce15a0' border=0 width=0 height=0 style='display:none'>