On Tue, 14 Nov 2017 12:23:55 -0600 Matt Hoosier <matt.hoos...@gmail.com> 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> Hi Matt, this patch looks very good. I have made some very small comments below, but I'm confident that those fixed or discussed this patch will be: Reviewed-by: Pekka Paalanen <pekka.paala...@collabora.co.uk> Everyone else, especially those in the CC, I would like to ask you to give your explicit Acked-by for this patch to record the community acceptance in the git history. Matt, could you hold off sending v4 for a while so that you can collect all the Reviewed-by's and Acked-by's that will arrive, but I am also fine collecting them myself if you want this sooner out of your hands. > --- > 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 > 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> > + > + <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. > </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; s/the Wayland display/a Wayland server/ perhaps? > + * 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. Good. > + * > * \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); There is no need to explicitly cast from void pointers. > + 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); This is ok, because test-runner.c already sets up a unique XDG_RUNTIME_DIR. I'm not sure the test is necessary, but it doesn't hurt. > + > + 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 I don't think it listens on the usual socket, because you don't call wl_display_add_socket{,_auto}(). > + * (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); Would be nice to check that the roundtrip actually succeeds. > + assert(client_display != NULL); The assert should be one line upwards. > + 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); > +} Thanks, pq
pgpCYiQdnVCe9.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
