[PATCH v3 wayland] client: Allow absolute paths in WAYLAND_DISPLAY
Matt Hoosier
matt.hoosier at gmail.com
Sat Nov 25 14:11:43 UTC 2017
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 <jadahl at gmail.com> wrote:
> 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.
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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list