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

Tue Nov 14 18:25:35 UTC 2017

On Tue, Nov 14, 2017 at 8:25 AM, Matt Hoosier <matt.hoosier at gmail.com> wrote:
> 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.
>>
>>>
>>>      <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/?
>>
>>> + * 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.
>>
>>> + *
>>>   * \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.
>
> There was as problem with implementing the regression test, that I
> wrote about above. Did you have any thoughts about getting around the
> problem where the malloc/free hooks in the test framework don't
> tolerate the irreclaimable allocations done by setenv()?

Nevermind; I hadn't noticed that you replied separately about that
issue. The latest patch version includes a test. Thanks.