[PATCH v3 wayland] client: Allow absolute paths in WAYLAND_DISPLAY
Pekka Paalanen
ppaalanen at gmail.com
Mon Nov 27 09:16:45 UTC 2017
On Sat, 25 Nov 2017 08:11:43 -0600
Matt Hoosier <matt.hoosier at gmail.com> wrote:
> 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?
Hi Matt,
you can downgrade my Reviewed-by to Acked-by in this case. I think
doing the changes Jonas suggested are good, but I cannot give R-b
without seeing the actual words. I'll just re-review then.
Thanks,
pq
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171127/b3cb7c1b/attachment-0001.sig>
More information about the wayland-devel
mailing list