[PATCH v2 wayland] client: Allow absolute paths in WAYLAND_DISPLAY

Tue Nov 14 18:26:59 UTC 2017

On Tue, Nov 14, 2017 at 8:22 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Fri, 10 Nov 2017 11:20:42 -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.
>>
>> 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>
>> ---
>>  doc/man/wl_display_connect.xml    | 22 +++++++++++++++++---
>>  doc/publican/sources/Protocol.xml |  5 ++++-
>>  src/wayland-client.c              | 43 +++++++++++++++++++++++++++++----------
>>  3 files changed, 55 insertions(+), 15 deletions(-)
>>
>> diff --git a/doc/man/wl_display_connect.xml b/doc/man/wl_display_connect.xml
>> index 7e6e05c..7bdfc46 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> 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>
>
> Hi Matt,
>
> this is a bit verbose, but it is correct. 'name' argument could now be
> absolute as well.

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/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..d90cbfb 100644
>> --- a/src/wayland-client.c
>> +++ b/src/wayland-client.c
>
> The code here is good.
>
>> @@ -994,6 +1009,12 @@ 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 \c NULL and the WAYLAND_DISPLAY environment variable
>> + * is set to an absolute pathname, then the pathname stored in
>> + * WAYLAND_DISPLAY is used immediately as the location of the
>
> s/immediately/as is/?

Done

>
>> + * socket at which the Wayland display is listening; no qualification
>> + * inside XDG_RUNTIME_DIR is attempted.
>
> There is one more case: 'name' itself can be an absolute path now.
>
> Would be good to mention the libwayland version where this changed here
> as well.

Done

>
>> + *
>>   * \memberof wl_display
>>   */
>>  WL_EXPORT struct wl_display *
>
> Looking pretty good to me, but I'll save my R-b for the revision with
> the test.

Done. That test should be available to you now.

Thanks for the reviews,
Matt