[PATCH wayland] client: Allow absolute paths in WAYLAND_DISPLAY

Fri Nov 10 11:37:15 UTC 2017

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.

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.

It would also be good to mention the version, that is, starting from
libwayland 1.15, WAYLAND_DISPLAY accepts also absolute paths.

>  
>      <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.

>  		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.)

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.

doc/publican/sources/Protocol.xml:96 briefly mentions WAYLAND_DISPLAY.
I wonder if that would also need more prose. Maybe not?

Thanks,
pq
-------------- 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/20171110/1944b83e/attachment.sig>