[PATCH wayland] client: Allow absolute paths in WAYLAND_DISPLAY
Matt Hoosier
matt.hoosier at gmail.com
Fri Nov 10 17:19:49 UTC 2017
Hi,
On Fri, Nov 10, 2017 at 5:37 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Thu, 9 Nov 2017 09:36:29 -0600
> Matt Hoosier <matt.hoosier at gmail.com> 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.
>
> Hi,
>
> Davide, let us know ASAP if you're not happy with the attribution.
>
> Matt, could add your Signed-off-by?
>
> Jonas' comment should be addressed. There are apps that do the
> $XDG_RUNTIME_DIR/$WAYLAND_DISPLAY concatenation themselves and do not
> expect an absolute path. They do that because they need the socket file
> path for e.g. bind-mounting it somewhere else, instead of just
> connecting to it.
Yeah, after a re-reading of Jonas's original message, I agree. That's
now implemented in v2.
>
> FWIW, if we had gone for the additional automatic fallback
> to /run/wayland/socket, those apps would still have the same problem.
> And even if we used a whole different environment variable for absolute
> paths.
>
>> ---
>> doc/man/wl_display_connect.xml | 11 ++++++++---
>> src/wayland-client.c | 34 +++++++++++++++++++++++-----------
>> 2 files changed, 31 insertions(+), 14 deletions(-)
>>
>> diff --git a/doc/man/wl_display_connect.xml b/doc/man/wl_display_connect.xml
>> index 7e6e05c..e86ee26 100644
>> --- a/doc/man/wl_display_connect.xml
>> +++ b/doc/man/wl_display_connect.xml
>> @@ -55,14 +55,19 @@
>> <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> 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 <envar>WAYLAND_DISPLAY</envar>
>> + is an absolute path then <varname>name</varname> is ignored and the
>> + path stored in <envar>WAYLAND_DISPLAY</envar> is used as the Wayland
>> + socket to which the connection is attempted.</para>
>
> WAYLAND_DISPLAY has never before overridden the function argument and I
> believe we should keep it that way. In fact, the code you propose
> already works like this, it's only the documentation here that is
> inaccurate.
Agreed, done.
>
> It would also be good to mention the version, that is, starting from
> libwayland 1.15, WAYLAND_DISPLAY accepts also absolute paths.
Done.
>
>>
>> <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/src/wayland-client.c b/src/wayland-client.c
>> index 3d7361e..2263d06 100644
>> --- a/src/wayland-client.c
>> +++ b/src/wayland-client.c
>> @@ -858,8 +858,13 @@ connect_to_socket(const char *name)
>> const char *runtime_dir;
>> int name_size, fd;
>>
>> + if (name == NULL)
>> + name = getenv("WAYLAND_DISPLAY");
>> + if (name == NULL)
>> + name = "wayland-0";
>> +
>> runtime_dir = getenv("XDG_RUNTIME_DIR");
>> - if (!runtime_dir) {
>> + if (!runtime_dir && (name[0] != '/')) {
>
> If you wanted to make the code more self-documenting, we could have e.g.
>
> bool path_is_abs;
>
> path_is_abs = name[0] == '/';
>
> and use that variable in the three places that test for absolute.
Might as well. Done.
>
>> wl_log("error: XDG_RUNTIME_DIR not set in the environment.\n");
>> /* to prevent programs reporting
>> * "failed to create display: Success" */
>> @@ -867,25 +872,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 (name[0] != '/') {
>> + 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 (name[0] != '/') {
>> + 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" */
>
> The code looks correct to me.
>
> You should also update the doxygen documentation for
> wl_display_connect() further down in this file. (I only just realized
> that we have duplicated the docs between doxygen and man.)
Done.
>
> One more thing to make this change perfect would be to add a test in
> the test suite to check that an absolute path works. It could live in
> socket-test.c maybe.
Writing the actual test wasn't very hard. But I ran aground on the
memory-leak checker. There are implicit allocations done by setenv()
calls in the individual test case to set $WAYLAND_DISPLAY to hold the
absolute path needed by the impending call to wl_display_connect(). I
couldn't find any guaranteed way to reclaim that memory -- unsetenv()
doesn't do the job.
If you have any thoughts on ways to get around this, I can add the test in.
>
> doc/publican/sources/Protocol.xml:96 briefly mentions WAYLAND_DISPLAY.
> I wonder if that would also need more prose. Maybe not?
Might as well. Done -- the new prose refers only to the 'reference
implementation' rather than claiming that this interpretation of
$WAYLAND_DISPLAY is guaranteed by every conceivable client program.
>
>
> Thanks,
> pq
Thanks for the review.
-Matt
More information about the wayland-devel
mailing list