Re: [PATCH wayland] extend WAYLAND_DISPLAY semantic and make server socket path configurable using environment
On 03/11/2015 07:05 PM, Bryce Harrington wrote: In any case, may as well check for error return from snprintf (i.e. name_size < 0). snprintf returns the number of bytes that would be needed, so truncation is indicated by returning a value greater than the size of the buffer. (yes a whole lot of old versions of Unix and Windows got this wrong, but I don't think Wayland ports to them anyway...) ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] extend WAYLAND_DISPLAY semantic and make server socket path configurable using environment
On Wed, Mar 11, 2015 at 10:05 PM, Bryce Harrington
wrote:
> On Tue, Feb 24, 2015 at 03:08:36PM +0100, Davide Bettio wrote:
> > commit e8b37de8e084d4e50a12bd2911657d54c0ebd9ed
> > Author: Davide Bettio
> > Date: Tue Feb 24 12:40:49 2015 +0100
> >
> > * Extend WAYLAND_DISPLAY and name parameter semantics to support
> > absolute paths.
> > For example WAYLAND_DISPLAY="/my/path/wayland-2" or
> > connect_to_socket("/my/path/wayland-2").
> >
> > * Introduce WAYLAND_SERVER_SOCKET_DIR to change socket directory
> > without changing XDG_RUNTIME_DIR.
> > This might be useful to change socket directory in case
> > wl_display_add_socket_auto is used.
> > For example this will change the socket path for weston without
> > using any command line argument.
> > This envvar is ignored when WAYLAND_SERVER_SOCKET is set to an
> > absolute path.
> >
> > * Introduce WAYLAND_SERVER_SOCKET to change the path where the
> > server will create the socket.
> > It will be possible for a nested compositor to offer a socket
> > located in WAYLAND_SERVER_SOCKET path
> > while connecting to the main compositor socket that is located
> > in WAYLAND_DISPLAY path.
> > That's the reason why a futher WAYLAND_SERVER_SOCKET envvar has
> > been introduced.
> >
> > Wayland clients will not see any new environment variable, while
> > compositors may have to deal
> > with 2 extra environment variables, this is due the fact that a
> > compositor might be a client of
> > another compositor and we must make sure that WAYLAND_DISPLAY is
> > not used for both client
> > and server purposes.
> >
> > Signed-off-by: Davide Bettio
> >
> > diff --git a/doc/man/wl_display_connect.xml
> > b/doc/man/wl_display_connect.xml
> > index 7e6e05c..b78c53f 100644
> > --- a/doc/man/wl_display_connect.xml
> > +++ b/doc/man/wl_display_connect.xml
> > @@ -59,10 +59,10 @@
> >find it. The name argument specifies
> > the name of
> >the socket or NULL to use the
> > default (which is
> >"wayland-0"). The environment variable
> > - WAYLAND_DISPLAY replaces the default value. If
> > - WAYLAND_SOCKET is set, this function
> > behaves like
> > - wl_display_connect_to_fd with the
> > file-descriptor
> > - number taken from the environment variable.
> > + WAYLAND_DISPLAY replaces the default
> > value, and eventually uses
> > + a different path too. If WAYLAND_SOCKET is
> > set, this function
> > + behaves like
> > wl_display_connect_to_fd with the
> > + file-descriptor number taken from the environment
> > variable.
> >
> > wl_display_connect_to_fd 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 0f1405c..cc01573 100644
> > --- a/src/wayland-client.c
> > +++ b/src/wayland-client.c
> > @@ -768,14 +768,26 @@ connect_to_socket(const char *name)
> >
> > 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 108 bytes\n", runtime_dir, name);
> > + } else {
> > + wl_log("error: socket path \"%s\" plus null
> terminator"
> > +" exceeds 108 bytes\n", name);
> > + }
>
> Perhaps instead of the 108 magic number in the log message, print the
> actual numerical value of (int)sizeof addr.sun_path ?
>
> > close(fd);
> > /* to prevent programs reporting
> >* "failed to add socket: Success" */
> > diff --git a/src/wayland-server.c b/src/wayland-server.c
> > index 0558634..39ba13f 100644
> > --- a/src/wayland-server.c
> > +++ b/src/wayland-server.c
> > @@ -1093,9 +1093,12 @@ wl_socket_init_for_display_name(struct
> > wl_socket *s, const char *name)
> > int name_size;
> > const char *runtime_dir;
> >
> > - runtime_dir = getenv("XDG_RUNTIME_DIR");
> > + runtime_dir = getenv("WA
Re: [PATCH wayland] extend WAYLAND_DISPLAY semantic and make server socket path configurable using environment
On Tue, Feb 24, 2015 at 03:08:36PM +0100, Davide Bettio wrote:
> commit e8b37de8e084d4e50a12bd2911657d54c0ebd9ed
> Author: Davide Bettio
> Date: Tue Feb 24 12:40:49 2015 +0100
>
> * Extend WAYLAND_DISPLAY and name parameter semantics to support
> absolute paths.
> For example WAYLAND_DISPLAY="/my/path/wayland-2" or
> connect_to_socket("/my/path/wayland-2").
>
> * Introduce WAYLAND_SERVER_SOCKET_DIR to change socket directory
> without changing XDG_RUNTIME_DIR.
> This might be useful to change socket directory in case
> wl_display_add_socket_auto is used.
> For example this will change the socket path for weston without
> using any command line argument.
> This envvar is ignored when WAYLAND_SERVER_SOCKET is set to an
> absolute path.
>
> * Introduce WAYLAND_SERVER_SOCKET to change the path where the
> server will create the socket.
> It will be possible for a nested compositor to offer a socket
> located in WAYLAND_SERVER_SOCKET path
> while connecting to the main compositor socket that is located
> in WAYLAND_DISPLAY path.
> That's the reason why a futher WAYLAND_SERVER_SOCKET envvar has
> been introduced.
>
> Wayland clients will not see any new environment variable, while
> compositors may have to deal
> with 2 extra environment variables, this is due the fact that a
> compositor might be a client of
> another compositor and we must make sure that WAYLAND_DISPLAY is
> not used for both client
> and server purposes.
>
> Signed-off-by: Davide Bettio
>
> diff --git a/doc/man/wl_display_connect.xml
> b/doc/man/wl_display_connect.xml
> index 7e6e05c..b78c53f 100644
> --- a/doc/man/wl_display_connect.xml
> +++ b/doc/man/wl_display_connect.xml
> @@ -59,10 +59,10 @@
>find it. The name argument specifies
> the name of
>the socket or NULL to use the
> default (which is
>"wayland-0"). The environment variable
> - WAYLAND_DISPLAY replaces the default value. If
> - WAYLAND_SOCKET is set, this function
> behaves like
> - wl_display_connect_to_fd with the
> file-descriptor
> - number taken from the environment variable.
> + WAYLAND_DISPLAY replaces the default
> value, and eventually uses
> + a different path too. If WAYLAND_SOCKET is
> set, this function
> + behaves like
> wl_display_connect_to_fd with the
> + file-descriptor number taken from the environment
> variable.
>
> wl_display_connect_to_fd 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 0f1405c..cc01573 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -768,14 +768,26 @@ connect_to_socket(const char *name)
>
> 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 108 bytes\n", runtime_dir, name);
> + } else {
> + wl_log("error: socket path \"%s\" plus null terminator"
> +" exceeds 108 bytes\n", name);
> + }
Perhaps instead of the 108 magic number in the log message, print the
actual numerical value of (int)sizeof addr.sun_path ?
> close(fd);
> /* to prevent programs reporting
>* "failed to add socket: Success" */
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 0558634..39ba13f 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1093,9 +1093,12 @@ wl_socket_init_for_display_name(struct
> wl_socket *s, const char *name)
> int name_size;
> const char *runtime_dir;
>
> - runtime_dir = getenv("XDG_RUNTIME_DIR");
> + runtime_dir = getenv("WAYLAND_SERVER_SOCKET_DIR");
> if (!runtime_dir) {
> - wl_log("error: XDG_RUNTIME_DIR not set in the environment\n");
> + runtime_dir = getenv("XDG_RUNTIME_DIR");
> + }
> + if (!runtime_dir) {
> + wl_log("error: XDG_RUNTIME_DIR or WAYLAND_
Re: [PATCH wayland] extend WAYLAND_DISPLAY semantic and make server socket path configurable using environment
Hi Davide,
This commit is actually several in one: can you please separate them?
When you do, please don't indent the commit message either.
On 24 February 2015 at 14:08, Davide Bettio wrote:
> * Extend WAYLAND_DISPLAY and name parameter semantics to support
> absolute paths.
> For example WAYLAND_DISPLAY="/my/path/wayland-2" or
> connect_to_socket("/my/path/wayland-2").
This would be commit #1.
> * Introduce WAYLAND_SERVER_SOCKET_DIR to change socket directory without
> changing XDG_RUNTIME_DIR.
> This might be useful to change socket directory in case
> wl_display_add_socket_auto is used.
> For example this will change the socket path for weston without using
> any command line argument.
> This envvar is ignored when WAYLAND_SERVER_SOCKET is set to an absolute
> path.
Commit #2.
> * Introduce WAYLAND_SERVER_SOCKET to change the path where the server
> will create the socket.
> It will be possible for a nested compositor to offer a socket located in
> WAYLAND_SERVER_SOCKET path
> while connecting to the main compositor socket that is located in
> WAYLAND_DISPLAY path.
> That's the reason why a futher WAYLAND_SERVER_SOCKET envvar has been
> introduced.
Commit #3.
> Wayland clients will not see any new environment variable, while
> compositors may have to deal
> with 2 extra environment variables, this is due the fact that a
> compositor might be a client of
> another compositor and we must make sure that WAYLAND_DISPLAY is not
> used for both client
> and server purposes.
Isn't this already the case today?
> 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 108 bytes\n", runtime_dir, name);
> + } else {
> + wl_log("error: socket path \"%s\" plus null
> terminator"
> + " exceeds 108 bytes\n", name);
> + }
I know this isn't actually your doing, but changing the hardcoded 108
to sizeof(addr.sun_path) at the same time would be nice.
> + } else {
> + //absolute path
C-style comments (/* */) please.
> + * If NULL is passed as name, then it would look in order for
> WAYLAND_SERVER_SOCKET
> + * and WAYLAND_SERVER
s/WAYLAND_SERVER/WAYLAND_DISPLAY/
> + * The Unix socket will be created in the directory pointed to by either
> environment
> + * variable WAYLAND_SERVER_SOCKET_DIR or XDG_RUNTIME_DIR. If the none of
> the is set,
> + * then this function fails and returns -1.
s/the none/neither/ (sorry, extreme nitpicking now)
> *
> - * The length of socket path, i.e., the path set in XDG_RUNTIME_DIR and the
> + * The length of socket path, e.g., the path set in XDG_RUNTIME_DIR and the
i.e. is correct - i.e. means 'that is' or 'specifically', whereas e.g.
means 'for example'.
With these comments addressed and the commit split:
Reviewed-by: Daniel Stone
Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
