Hi Jonas, Pekka, I have no objection to making the tweaks that Jonas mentions, but I'm wary of messing with the form of this patch that Pekka already stamped. Pekka, am I going to lose your consent as given in v3 if I make changes along the lines Jonas requests?
On Tue, Nov 21, 2017 at 10:00 PM, Jonas Ådahl <jad...@gmail.com> wrote: > On Tue, Nov 14, 2017 at 12:23:55PM -0600, Matt Hoosier wrote: >> From: Matt Hoosier <matt.hoos...@garmin.com> >> >> In order to support system compositor instances, it is necessary to >> allow clients' wl_display_connect() to find the compositor's listening >> socket somewhere outside of XDG_RUNTIME_DIR. For a full account, see >> the discussion beginning here: >> >> https://lists.freedesktop.org/archives/wayland-devel/2017-November/035664.html >> >> This change adjusts the client-side connection logic so that, if >> WAYLAND_DISPLAY is formatted as an absolute pathname, the socket >> connection attempt is made to just $WAYLAND_DISPLAY rather than >> usual user-private location $XDG_RUNTIME_DIR/$WAYLAND_DISPLAY. >> >> This change is based on Davide Bettio's submission of the same concept >> at: >> >> https://lists.freedesktop.org/archives/wayland-devel/2015-August/023838.html. >> >> v3 changes: >> >> * Added test case. >> * Clarified documentation to note that 'name' parameter to >> wl_display_connect() >> can also be an absolute path. >> >> v2 changes: >> >> * Added backward incompatibility note to wl_display_connect() manpage. >> * Rephased wl_display_connect() manpage changes to precisely match actual >> changed behavior. >> * Added mention of new absolute path behavior in wl_display_connect() >> doxygen comments. >> * Mentioned new absolute path interpretation of WAYLAND_DISPLAY in >> protocol documentation. >> >> Signed-off-by: Matt Hoosier <matt.hoos...@gmail.com> > > This feature is > > Acked-by: Jonas Ådahl <jad...@gmail.com> > > but I have a few comments on the documentation below. > >> --- >> doc/man/wl_display_connect.xml | 22 ++++++-- >> doc/publican/sources/Protocol.xml | 5 +- >> src/wayland-client.c | 47 ++++++++++++---- >> tests/socket-test.c | 109 >> ++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 168 insertions(+), 15 deletions(-) >> >> diff --git a/doc/man/wl_display_connect.xml b/doc/man/wl_display_connect.xml >> index 7e6e05c..9c67612 100644 >> --- a/doc/man/wl_display_connect.xml >> +++ b/doc/man/wl_display_connect.xml >> @@ -55,14 +55,30 @@ >> <title>Description</title> >> <para><function>wl_display_connect</function> connects to a Wayland >> socket >> that was previously opened by a Wayland server. The server socket >> must >> - be placed in <envar>XDG_RUNTIME_DIR</envar> for this function to >> - find it. The <varname>name</varname> argument specifies the name >> of >> + be placed in <envar>XDG_RUNTIME_DIR</envar> or exist at the >> absolute >> + path referred to by <envar>WAYLAND_DISPLAY</envar> or >> <varname>name</varname> >> + for this function to find it. The <varname>name</varname> >> argument specifies the name of > > nit1: This line looks like it should be split. > > nit2: It's easy to miss the compatibility issues by not mentioning > anything about it here, so maybe refer to the section below here? > Something like > > The server socket must be placed in <envvar>XDG_RUNTIME_DIR</envvar> > or, depending on version libwayland, exist at the absolute path > referred to by <envar>WAYLA... ...find it. See below for compatibility > issue details. > > Otherwise, I think the new wording is a bit awkward: > >> <para><function>wl_display_connect</function> connects to a Wayland >> socket >> that was previously opened by a Wayland server. The server socket >> must >> be placed in <envar>XDG_RUNTIME_DIR</envar> or exist at the >> absolute >> path referred to by <envar>WAYLAND_DISPLAY</envar> or >> <varname>name</varname> >> for this function to find it. > > This changes the meaning of "name" and WAYLAND_DISPLAY. Can you be more specific? The implementation does allow 'name' now to encode an absolute path, so I agree that it changes the meaning in that sense. What else did you mean? The bit saying 'the server socket must be placed in XDG_RUNTIME_DIR' is recycled language that already existed before this change. > >> The <varname>name</varname> argument specifies the name of >> the socket or <constant>NULL</constant> to use the default (which >> is >> <constant>"wayland-0"</constant>). > > This refers to the old meaning where "name" is either NULL or the socket > *name*. > >> The environment variable >> <envar>WAYLAND_DISPLAY</envar> replaces the default value. > > It makes the the new documentation a bit confusing. > > >> the socket or <constant>NULL</constant> to use the default (which >> is >> <constant>"wayland-0"</constant>). The environment variable >> <envar>WAYLAND_DISPLAY</envar> replaces the default value. If >> <envar>WAYLAND_SOCKET</envar> is set, this function behaves like >> <function>wl_display_connect_to_fd</function> with the >> file-descriptor >> - number taken from the environment variable.</para> >> + number taken from the environment variable. If >> + <envar>WAYLAND_SOCKET</envar> is not set and >> <varname>name</varname> >> + is <constant>NULL</constant> and <envar>WAYLAND_DISPLAY</envar> >> + is an absolute path, then the path stored in >> <envar>WAYLAND_DISPLAY</envar> >> + is used as the Wayland socket to which the connection is >> attempted.</para> > > Isn't this change redundant? If the first change amendment the documentation > redefines the meaning of <varname>name</varname> and > <envvar>WAYLAND_DISPLAY</envvar> we don't need to repeat half of the semantics > here. I'm having a hard time judging what redundancies are wanted and which aren't in the documentation. For example, there are three independent sets of notes for wl_display_connect(): the doxygen stuff, the manpage, and the publican materials. Is it the intra-document repetition (the manpage, here) that you're not liking? > >> + >> + <para>Support for interpreting <envar>WAYLAND_DISPLAY</envar> as an >> + absolute path is a change in behavior compared to >> + <function>wl_display_connect</function>'s behavior in versions >> + 1.14 and older of Wayland. It is no longer guaranteed in versions >> + 1.15 and higher that the Wayland socket chosen is equivalent to >> + manually constructing a socket pathname by concatenating >> + <envar>XDG_RUNTIME_DIR</envar> and <envar>WAYLAND_DISPLAY</envar>. >> + Manual construction of the socket path must account for the >> + possibility that <envar>WAYLAND_DISPLAY</envar> contains an >> absolute >> + path.</para> >> >> <para><function>wl_display_connect_to_fd</function> connects to a >> Wayland >> socket with an explicit file-descriptor. The file-descriptor is >> passed >> diff --git a/doc/publican/sources/Protocol.xml >> b/doc/publican/sources/Protocol.xml >> index ba6b5f1..dbfed06 100644 >> --- a/doc/publican/sources/Protocol.xml >> +++ b/doc/publican/sources/Protocol.xml >> @@ -94,7 +94,10 @@ >> The protocol is sent over a UNIX domain stream socket, where the >> endpoint >> usually is named <systemitem class="service">wayland-0</systemitem> >> (although it can be changed via <emphasis>WAYLAND_DISPLAY</emphasis> >> - in the environment). >> + in the environment). In the reference implementation, a client whose >> + <emphasis>WAYLAND_DISPLAY</emphasis> is formatted as an absolute path >> + connects to that path as the endpoint, otherwise the implementation >> + searches in <emphasis>XDG_RUNTIME_DIR</emphasis> for the endpoint. > > The documentation amended earlier in this patch updates the > wl_display_connect man > page, and contains the compatibility issues, but this part, updating the > protocol documentation does not contain this information. I'm not sure > referring > to any reference implementation here makes up for this either (or that we even > should). Just to be clear: I think you're calling for either complete silence on what the reference implementation does, or else equal detail about such-and-such version of the reference implementation that first supports absolute paths. Is that right? > > From a purely protocol documentation point of view, I think it should be made > clear that an implementation can *optionally* support absolute paths; > otherwise > libwayland 1.14 and older suddenly are not compliant anymore. That's why I didn't write any obligations about the protocol itself. Would you prefer that the protocol docs require implementors to support absolute paths in $WAYLAND_DISPLAY? > > > Jonas > >> </para> >> <para> >> Every message is structured as 32-bit words; values are represented >> in the >> diff --git a/src/wayland-client.c b/src/wayland-client.c >> index 3d7361e..bcf35a6 100644 >> --- a/src/wayland-client.c >> +++ b/src/wayland-client.c >> @@ -857,9 +857,17 @@ connect_to_socket(const char *name) >> socklen_t size; >> const char *runtime_dir; >> int name_size, fd; >> + bool path_is_absolute; >> + >> + if (name == NULL) >> + name = getenv("WAYLAND_DISPLAY"); >> + if (name == NULL) >> + name = "wayland-0"; >> + >> + path_is_absolute = name[0] == '/'; >> >> runtime_dir = getenv("XDG_RUNTIME_DIR"); >> - if (!runtime_dir) { >> + if (!runtime_dir && !path_is_absolute) { >> wl_log("error: XDG_RUNTIME_DIR not set in the environment.\n"); >> /* to prevent programs reporting >> * "failed to create display: Success" */ >> @@ -867,25 +875,32 @@ connect_to_socket(const char *name) >> return -1; >> } >> >> - if (name == NULL) >> - name = getenv("WAYLAND_DISPLAY"); >> - if (name == NULL) >> - name = "wayland-0"; >> - >> fd = wl_os_socket_cloexec(PF_LOCAL, SOCK_STREAM, 0); >> if (fd < 0) >> return -1; >> >> memset(&addr, 0, sizeof addr); >> addr.sun_family = AF_LOCAL; >> - name_size = >> - snprintf(addr.sun_path, sizeof addr.sun_path, >> - "%s/%s", runtime_dir, name) + 1; >> + if (!path_is_absolute) { >> + name_size = >> + snprintf(addr.sun_path, sizeof addr.sun_path, >> + "%s/%s", runtime_dir, name) + 1; >> + } else { >> + /* absolute path */ >> + name_size = >> + snprintf(addr.sun_path, sizeof addr.sun_path, >> + "%s", name) + 1; >> + } >> >> assert(name_size > 0); >> if (name_size > (int)sizeof addr.sun_path) { >> - wl_log("error: socket path \"%s/%s\" plus null terminator" >> - " exceeds 108 bytes\n", runtime_dir, name); >> + if (!path_is_absolute) { >> + wl_log("error: socket path \"%s/%s\" plus null >> terminator" >> + " exceeds %i bytes\n", runtime_dir, name, (int) >> sizeof(addr.sun_path)); >> + } else { >> + wl_log("error: socket path \"%s\" plus null terminator" >> + " exceeds %i bytes\n", name, (int) >> sizeof(addr.sun_path)); >> + } >> close(fd); >> /* to prevent programs reporting >> * "failed to add socket: Success" */ >> @@ -994,6 +1009,16 @@ wl_display_connect_to_fd(int fd) >> * its value will be replaced with the WAYLAND_DISPLAY environment >> * variable if it is set, otherwise display "wayland-0" will be used. >> * >> + * If \c name is an absolute path, then that path is used as-is for >> + * the location of the socket at which the Wayland display is listening; >> + * no qualification inside XDG_RUNTIME_DIR is attempted. >> + * >> + * If \c name is \c NULL and the WAYLAND_DISPLAY environment variable >> + * is set to an absolute pathname, then that pathname is used as-is >> + * for the socket in the same manner as if \c name held an absolute >> + * path. Support for absolute paths in \c name and WAYLAND_DISPLAY >> + * is present since Wayland version 1.15. >> + * >> * \memberof wl_display >> */ >> WL_EXPORT struct wl_display * >> diff --git a/tests/socket-test.c b/tests/socket-test.c >> index bb034f4..427cf80 100644 >> --- a/tests/socket-test.c >> +++ b/tests/socket-test.c >> @@ -26,10 +26,14 @@ >> #include <errno.h> >> #include <string.h> >> #include <stdio.h> >> +#include <sys/socket.h> >> +#include <sys/types.h> >> +#include <sys/wait.h> >> #include <sys/un.h> >> #include <unistd.h> >> >> #include "wayland-client.h" >> +#include "wayland-os.h" >> #include "wayland-server.h" >> #include "test-runner.h" >> >> @@ -173,3 +177,108 @@ TEST(add_socket_auto) >> >> wl_display_destroy(d); >> } >> + >> +struct client_create_listener { >> + struct wl_listener listener; >> + struct wl_display *display; >> +}; >> + >> +struct client_destroy_listener { >> + struct wl_listener listener; >> + struct wl_display *display; >> +}; >> + >> +static void >> +client_destroy_notify(struct wl_listener *l, void *data) >> +{ >> + struct client_destroy_listener *listener = >> + wl_container_of(l, listener, listener); >> + wl_display_terminate(listener->display); >> + free(listener); >> +} >> + >> +static void >> +client_create_notify(struct wl_listener *l, void *data) >> +{ >> + struct wl_client *client = (struct wl_client *)data; >> + struct client_create_listener *listener = >> + wl_container_of(l, listener, listener); >> + struct client_destroy_listener *destroy_listener = (struct >> client_destroy_listener *)malloc(sizeof *destroy_listener); >> + assert(destroy_listener != NULL); >> + destroy_listener->display = listener->display; >> + destroy_listener->listener.notify = client_destroy_notify; >> + wl_client_add_destroy_listener(client, &destroy_listener->listener); >> +} >> + >> +TEST(absolute_socket_path) >> +{ >> + struct wl_display *display; >> + struct client_create_listener client_create_listener; >> + struct sockaddr_un addr; >> + int fd; >> + socklen_t size; >> + const char *xdg_runtime_dir; >> + size_t len; >> + int ret; >> + pid_t pid; >> + >> + /* It's a little weird that this test about absolute socket paths >> + * uses XDG_RUNTIME_DIR, but that's the only location guaranteed >> + * by test-runner to be both writable and unique. This isn't >> + * really a problem; we'll just take care that the leaf-level >> + * filename used for the socket isn't anything that would >> + * accidentally be generated by a default usage of >> wl_display_connect(). */ >> + xdg_runtime_dir = require_xdg_runtime_dir(); >> + memset(&addr, 0, sizeof addr); >> + len = snprintf(addr.sun_path, sizeof addr.sun_path, >> + "%s/%s", xdg_runtime_dir, "wayland-absolute-0"); >> + assert(len < sizeof addr.sun_path >> + && "Bug in test. Path too long"); >> + >> + /* The path must not exist prior to binding. */ >> + assert(access(addr.sun_path, F_OK) == -1); >> + >> + size = offsetof (struct sockaddr_un, sun_path) + strlen(addr.sun_path); >> + addr.sun_family = AF_LOCAL; >> + fd = wl_os_socket_cloexec(PF_LOCAL, SOCK_STREAM, 0); >> + assert(fd >= 0 ); >> + ret = bind(fd, (struct sockaddr *) &addr, size); >> + assert(ret >= 0); >> + ret = listen(fd, 128); >> + assert(ret >= 0); >> + >> + /* Start server display. It'll listen both on the usual >> $XDG_RUNTIME_DIR/wayland-0 >> + * (unused for this test) and the supplementary absolutely qualified >> socket >> + * made above. */ >> + display = wl_display_create(); >> + assert(display != NULL); >> + client_create_listener.listener.notify = client_create_notify; >> + client_create_listener.display = display; >> + wl_display_add_client_created_listener(display, >> &client_create_listener.listener); >> + ret = wl_display_add_socket_fd(display, fd); >> + assert(ret == 0); >> + >> + /* Execute client that connects to the absolutely qualified server >> socket path. */ >> + pid = fork(); >> + assert(pid != -1); >> + >> + if (pid == 0) { >> + ret = setenv("WAYLAND_DISPLAY", addr.sun_path, 1); >> + assert(ret == 0); >> + struct wl_display *client_display = wl_display_connect(NULL); >> + wl_display_roundtrip(client_display); >> + assert(client_display != NULL); >> + wl_display_disconnect(client_display); >> + exit(0); >> + assert(false); >> + } >> + >> + wl_display_run(display); >> + ret = waitpid(pid, NULL, 0); >> + assert(ret == pid); >> + >> + wl_display_destroy(display); >> + >> + ret = unlink(addr.sun_path); >> + assert(ret == 0); >> +} >> -- >> 2.13.6 >> >> _______________________________________________ >> wayland-devel mailing list >> wayland-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
