On Sat, 25 Nov 2017 08:11:43 -0600 Matt Hoosier <matt.hoos...@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 <jad...@gmail.com> wrote: > > On Tue, Nov 14, 2017 at 12:23:55PM -0600, Matt Hoosier wrote: > >> From: Matt Hoosier <matt.hoos...@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.hoos...@gmail.com> > > > > This feature is > > > > Acked-by: Jonas Ådahl <jad...@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
pgp6w_C1E61ud.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
