[PATCH v3 wayland] client: Allow absolute paths in WAYLAND_DISPLAY
Jonas Ådahl
jadahl at gmail.com
Wed Nov 22 04:00:27 UTC 2017
On Tue, Nov 14, 2017 at 12:23:55PM -0600, Matt Hoosier wrote:
> From: Matt Hoosier <matt.hoosier at 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.hoosier at gmail.com>
This feature is
Acked-by: Jonas Ådahl <jadahl at 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.
> 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.
> +
> + <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).
>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.
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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list