[2/2] support specifying custom directories for the client and server

Submitted by Imran Zaman on Oct. 15, 2014, 2:36 p.m.

Details

Message ID CAPfuZRgje1gZY2D5uOmPdXeCD+yQJaUrvOYomjORe5LaPb-2kw@mail.gmail.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Imran Zaman Oct. 15, 2014, 2:36 p.m.
Hi

support for adjusting socket access rights to allow group of users to
connect to the socket.

This is used for nested compositor architectures.
-----

  if (runtime_dir == NULL)
@@ -1133,6 +1139,18 @@ _wl_display_add_socket(struct wl_display
*display, struct wl_socket *s)
  return -1;
  }

+ socket_group_str = getenv("WAYLAND_SERVER_GROUP");
+ if (socket_group_str != NULL) {
+ socket_group = getgrnam(socket_group_str);
+ if (socket_group != NULL)
+ chown(s->addr.sun_path, -1, socket_group->gr_gid);
+ }
+ socket_mode_str = getenv("WAYLAND_SERVER_MODE");
+ if (socket_mode_str != NULL) {
+ if (sscanf(socket_mode_str, "%o", &socket_mode) > 0)
+ chmod(s->addr.sun_path, socket_mode);
+ }
+
  s->source = wl_event_loop_add_fd(display->loop, s->fd,
  WL_EVENT_READABLE,
  socket_data, display);

Patch hide | download patch | download mbox

diff --git a/src/wayland-server.c b/src/wayland-server.c
index ce1eca8..b1ca5e6 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -39,6 +39,8 @@ 
 #include <fcntl.h>
 #include <sys/file.h>
 #include <sys/stat.h>
+#include <sys/types.h>
+#include <grp.h>
 #include <ffi.h>

 #include "wayland-private.h"
@@ -1079,6 +1081,10 @@  wl_socket_init_for_display_name(struct
wl_socket *s, const char *name)
 {
  int name_size;
  const char *runtime_dir;
+ const char *socket_mode_str;
+ const char *socket_group_str;
+ const struct group *socket_group;
+ unsigned socket_mode;

  runtime_dir = getenv("WAYLAND_SERVER_DIR");

Comments

On Wed, Oct 15, 2014 at 05:36:27PM +0300, Imran Zaman wrote:
> Hi
> 
> support for adjusting socket access rights to allow group of users to
> connect to the socket.
> 
> This is used for nested compositor architectures.
> -----
> 
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index ce1eca8..b1ca5e6 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -39,6 +39,8 @@
>  #include <fcntl.h>
>  #include <sys/file.h>
>  #include <sys/stat.h>
> +#include <sys/types.h>
> +#include <grp.h>
>  #include <ffi.h>
> 
>  #include "wayland-private.h"
> @@ -1079,6 +1081,10 @@ wl_socket_init_for_display_name(struct
> wl_socket *s, const char *name)
>  {
>   int name_size;
>   const char *runtime_dir;
> + const char *socket_mode_str;
> + const char *socket_group_str;
> + const struct group *socket_group;
> + unsigned socket_mode;
> 
>   runtime_dir = getenv("WAYLAND_SERVER_DIR");
>   if (runtime_dir == NULL)
> @@ -1133,6 +1139,18 @@ _wl_display_add_socket(struct wl_display
> *display, struct wl_socket *s)
>   return -1;
>   }
> 

The tabbing seems to be off in the following chunk of code.  Maybe got
eaten when inlining the patch?

> + socket_group_str = getenv("WAYLAND_SERVER_GROUP");
> + if (socket_group_str != NULL) {
> + socket_group = getgrnam(socket_group_str);
> + if (socket_group != NULL)
> + chown(s->addr.sun_path, -1, socket_group->gr_gid);
> + }
> + socket_mode_str = getenv("WAYLAND_SERVER_MODE");
> + if (socket_mode_str != NULL) {
> + if (sscanf(socket_mode_str, "%o", &socket_mode) > 0)
> + chmod(s->addr.sun_path, socket_mode);
> + }

Return values should be checked on the chown and chmod.

Other than that, for both patches:

Reviewed-by: Bryce Harrington <b.harrington@samsung.com>

> +
>   s->source = wl_event_loop_add_fd(display->loop, s->fd,
>   WL_EVENT_READABLE,
>   socket_data, display);
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
On Wed, 15 Oct 2014 17:36:27 +0300
Imran Zaman <imran.zaman@gmail.com> wrote:

> Hi
> 
> support for adjusting socket access rights to allow group of users to
> connect to the socket.
> 
> This is used for nested compositor architectures.
> -----
> 
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index ce1eca8..b1ca5e6 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -39,6 +39,8 @@
>  #include <fcntl.h>
>  #include <sys/file.h>
>  #include <sys/stat.h>
> +#include <sys/types.h>
> +#include <grp.h>
>  #include <ffi.h>
> 
>  #include "wayland-private.h"
> @@ -1079,6 +1081,10 @@ wl_socket_init_for_display_name(struct
> wl_socket *s, const char *name)
>  {
>   int name_size;
>   const char *runtime_dir;
> + const char *socket_mode_str;
> + const char *socket_group_str;
> + const struct group *socket_group;
> + unsigned socket_mode;
> 
>   runtime_dir = getenv("WAYLAND_SERVER_DIR");
>   if (runtime_dir == NULL)
> @@ -1133,6 +1139,18 @@ _wl_display_add_socket(struct wl_display
> *display, struct wl_socket *s)
>   return -1;
>   }
> 
> + socket_group_str = getenv("WAYLAND_SERVER_GROUP");
> + if (socket_group_str != NULL) {
> + socket_group = getgrnam(socket_group_str);
> + if (socket_group != NULL)
> + chown(s->addr.sun_path, -1, socket_group->gr_gid);
> + }
> + socket_mode_str = getenv("WAYLAND_SERVER_MODE");
> + if (socket_mode_str != NULL) {
> + if (sscanf(socket_mode_str, "%o", &socket_mode) > 0)
> + chmod(s->addr.sun_path, socket_mode);
> + }
> +
>   s->source = wl_event_loop_add_fd(display->loop, s->fd,
>   WL_EVENT_READABLE,
>   socket_data, display);

Hi,

I'm not sure what to think of these two patches. Are environment
variables WAYLAND_CLIENT_DIR, WAYLAND_SERVER_DIR, and the above really
the right API for this?

Your system compositor has to be specially written to be a system
compositor and open multiple listening sockets anyway, so I think this
is the wrong way for the server side stuff. For the server side, I
don't think we should have anything of this controlled via environment
variables but new API functions. What those API functions should be is
a good question.

I believe you don't need any additions in libwayland-server, in fact.
You have very special requirements in how and where to create the
sockets, so you can do that all yourself, up to the point where you have
a socket fd listening for new connections. Then you can use
wl_event_loop_add_fd() (if you even use the libwayland-server event
loops stuff) to monitor the fd. When a client connects, you get a
callback, call accept(), and pass the new fd to wl_client_create().

Hmm, wait, why am I assuming multiple listening sockets... What is your
architecture here, exactly?

I have very hard time deciding if we should allow the environment to
overwrite the server and client assumptions on where the socket is. It
would be easier for me to accept new API functions that operate with
absolute paths or existing fds explicitly, but those of course require
both servers and clients to be written to use them.

Comments, anyone?


Thanks,
pq
... corrected jussi email address..

BR
irman

On Wed, Nov 19, 2014 at 12:56 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Wed, 15 Oct 2014 17:36:27 +0300
> Imran Zaman <imran.zaman@gmail.com> wrote:
>
>> Hi
>>
>> support for adjusting socket access rights to allow group of users to
>> connect to the socket.
>>
>> This is used for nested compositor architectures.
>> -----
>>
>> diff --git a/src/wayland-server.c b/src/wayland-server.c
>> index ce1eca8..b1ca5e6 100644
>> --- a/src/wayland-server.c
>> +++ b/src/wayland-server.c
>> @@ -39,6 +39,8 @@
>>  #include <fcntl.h>
>>  #include <sys/file.h>
>>  #include <sys/stat.h>
>> +#include <sys/types.h>
>> +#include <grp.h>
>>  #include <ffi.h>
>>
>>  #include "wayland-private.h"
>> @@ -1079,6 +1081,10 @@ wl_socket_init_for_display_name(struct
>> wl_socket *s, const char *name)
>>  {
>>   int name_size;
>>   const char *runtime_dir;
>> + const char *socket_mode_str;
>> + const char *socket_group_str;
>> + const struct group *socket_group;
>> + unsigned socket_mode;
>>
>>   runtime_dir = getenv("WAYLAND_SERVER_DIR");
>>   if (runtime_dir == NULL)
>> @@ -1133,6 +1139,18 @@ _wl_display_add_socket(struct wl_display
>> *display, struct wl_socket *s)
>>   return -1;
>>   }
>>
>> + socket_group_str = getenv("WAYLAND_SERVER_GROUP");
>> + if (socket_group_str != NULL) {
>> + socket_group = getgrnam(socket_group_str);
>> + if (socket_group != NULL)
>> + chown(s->addr.sun_path, -1, socket_group->gr_gid);
>> + }
>> + socket_mode_str = getenv("WAYLAND_SERVER_MODE");
>> + if (socket_mode_str != NULL) {
>> + if (sscanf(socket_mode_str, "%o", &socket_mode) > 0)
>> + chmod(s->addr.sun_path, socket_mode);
>> + }
>> +
>>   s->source = wl_event_loop_add_fd(display->loop, s->fd,
>>   WL_EVENT_READABLE,
>>   socket_data, display);
>
> Hi,
>
> I'm not sure what to think of these two patches. Are environment
> variables WAYLAND_CLIENT_DIR, WAYLAND_SERVER_DIR, and the above really
> the right API for this?
>
> Your system compositor has to be specially written to be a system
> compositor and open multiple listening sockets anyway, so I think this
> is the wrong way for the server side stuff. For the server side, I
> don't think we should have anything of this controlled via environment
> variables but new API functions. What those API functions should be is
> a good question.
>
> I believe you don't need any additions in libwayland-server, in fact.
> You have very special requirements in how and where to create the
> sockets, so you can do that all yourself, up to the point where you have
> a socket fd listening for new connections. Then you can use
> wl_event_loop_add_fd() (if you even use the libwayland-server event
> loops stuff) to monitor the fd. When a client connects, you get a
> callback, call accept(), and pass the new fd to wl_client_create().
>
> Hmm, wait, why am I assuming multiple listening sockets... What is your
> architecture here, exactly?
>
> I have very hard time deciding if we should allow the environment to
> overwrite the server and client assumptions on where the socket is. It
> would be easier for me to accept new API functions that operate with
> absolute paths or existing fds explicitly, but those of course require
> both servers and clients to be written to use them.
>
> Comments, anyone?
>
>
> Thanks,
> pq
Hi,

maybe I just don't understand how this should work...

You have system compositor which spawns some other wayland compositors?
Then you can specify these (or any other) environment vars in the
compositor, the child compositors will inherit these...


On 19 November 2014 12:52, Imran Zaman <imran.zaman@gmail.com> wrote:

> ... corrected jussi email address..
>
> BR
> irman
>
> On Wed, Nov 19, 2014 at 12:56 PM, Pekka Paalanen <ppaalanen@gmail.com>
> wrote:
> > On Wed, 15 Oct 2014 17:36:27 +0300
> > Imran Zaman <imran.zaman@gmail.com> wrote:
> >
> >> Hi
> >>
> >> support for adjusting socket access rights to allow group of users to
> >> connect to the socket.
> >>
> >> This is used for nested compositor architectures.
> >> -----
> >>
> >> diff --git a/src/wayland-server.c b/src/wayland-server.c
> >> index ce1eca8..b1ca5e6 100644
> >> --- a/src/wayland-server.c
> >> +++ b/src/wayland-server.c
> >> @@ -39,6 +39,8 @@
> >>  #include <fcntl.h>
> >>  #include <sys/file.h>
> >>  #include <sys/stat.h>
> >> +#include <sys/types.h>
> >> +#include <grp.h>
> >>  #include <ffi.h>
> >>
> >>  #include "wayland-private.h"
> >> @@ -1079,6 +1081,10 @@ wl_socket_init_for_display_name(struct
> >> wl_socket *s, const char *name)
> >>  {
> >>   int name_size;
> >>   const char *runtime_dir;
> >> + const char *socket_mode_str;
> >> + const char *socket_group_str;
> >> + const struct group *socket_group;
> >> + unsigned socket_mode;
> >>
> >>   runtime_dir = getenv("WAYLAND_SERVER_DIR");
> >>   if (runtime_dir == NULL)
> >> @@ -1133,6 +1139,18 @@ _wl_display_add_socket(struct wl_display
> >> *display, struct wl_socket *s)
> >>   return -1;
> >>   }
> >>
> >> + socket_group_str = getenv("WAYLAND_SERVER_GROUP");
> >> + if (socket_group_str != NULL) {
> >> + socket_group = getgrnam(socket_group_str);
> >> + if (socket_group != NULL)
> >> + chown(s->addr.sun_path, -1, socket_group->gr_gid);
> >> + }
> >> + socket_mode_str = getenv("WAYLAND_SERVER_MODE");
> >> + if (socket_mode_str != NULL) {
> >> + if (sscanf(socket_mode_str, "%o", &socket_mode) > 0)
> >> + chmod(s->addr.sun_path, socket_mode);
> >> + }
> >> +
> >>   s->source = wl_event_loop_add_fd(display->loop, s->fd,
> >>   WL_EVENT_READABLE,
> >>   socket_data, display);
> >
> > Hi,
> >
> > I'm not sure what to think of these two patches. Are environment
> > variables WAYLAND_CLIENT_DIR, WAYLAND_SERVER_DIR, and the above really
> > the right API for this?
> >
> > Your system compositor has to be specially written to be a system
> > compositor and open multiple listening sockets anyway, so I think this
> > is the wrong way for the server side stuff. For the server side, I
> > don't think we should have anything of this controlled via environment
> > variables but new API functions. What those API functions should be is
> > a good question.
> >
> > I believe you don't need any additions in libwayland-server, in fact.
> > You have very special requirements in how and where to create the
> > sockets, so you can do that all yourself, up to the point where you have
> > a socket fd listening for new connections. Then you can use
> > wl_event_loop_add_fd() (if you even use the libwayland-server event
> > loops stuff) to monitor the fd. When a client connects, you get a
> > callback, call accept(), and pass the new fd to wl_client_create().
> >
> > Hmm, wait, why am I assuming multiple listening sockets... What is your
> > architecture here, exactly?
> >
> > I have very hard time deciding if we should allow the environment to
> > overwrite the server and client assumptions on where the socket is. It
> > would be easier for me to accept new API functions that operate with
> > absolute paths or existing fds explicitly, but those of course require
> > both servers and clients to be written to use them.
> >
> > Comments, anyone?
> >
> >
> > Thanks,
> > pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
On Wed, 19 Nov 2014 14:14:34 +0100
Marek Chalupa <mchqwerty@gmail.com> wrote:

> Hi,
> 
> maybe I just don't understand how this should work...
> 
> You have system compositor which spawns some other wayland compositors?
> Then you can specify these (or any other) environment vars in the
> compositor, the child compositors will inherit these...
> 
> 
> On 19 November 2014 12:52, Imran Zaman <imran.zaman@gmail.com> wrote:
> 
> > ... corrected jussi email address..
> >
> > BR
> > irman
> >
> > On Wed, Nov 19, 2014 at 12:56 PM, Pekka Paalanen <ppaalanen@gmail.com>
> > wrote:
> > > On Wed, 15 Oct 2014 17:36:27 +0300
> > > Imran Zaman <imran.zaman@gmail.com> wrote:
> > >
> > >> Hi
> > >>
> > >> support for adjusting socket access rights to allow group of users to
> > >> connect to the socket.
> > >>
> > >> This is used for nested compositor architectures.
> > >> -----
> > >>
> > >> diff --git a/src/wayland-server.c b/src/wayland-server.c
> > >> index ce1eca8..b1ca5e6 100644
> > >> --- a/src/wayland-server.c
> > >> +++ b/src/wayland-server.c
> > >> @@ -39,6 +39,8 @@
> > >>  #include <fcntl.h>
> > >>  #include <sys/file.h>
> > >>  #include <sys/stat.h>
> > >> +#include <sys/types.h>
> > >> +#include <grp.h>
> > >>  #include <ffi.h>
> > >>
> > >>  #include "wayland-private.h"
> > >> @@ -1079,6 +1081,10 @@ wl_socket_init_for_display_name(struct
> > >> wl_socket *s, const char *name)
> > >>  {
> > >>   int name_size;
> > >>   const char *runtime_dir;
> > >> + const char *socket_mode_str;
> > >> + const char *socket_group_str;
> > >> + const struct group *socket_group;
> > >> + unsigned socket_mode;
> > >>
> > >>   runtime_dir = getenv("WAYLAND_SERVER_DIR");
> > >>   if (runtime_dir == NULL)
> > >> @@ -1133,6 +1139,18 @@ _wl_display_add_socket(struct wl_display
> > >> *display, struct wl_socket *s)
> > >>   return -1;
> > >>   }
> > >>
> > >> + socket_group_str = getenv("WAYLAND_SERVER_GROUP");
> > >> + if (socket_group_str != NULL) {
> > >> + socket_group = getgrnam(socket_group_str);
> > >> + if (socket_group != NULL)
> > >> + chown(s->addr.sun_path, -1, socket_group->gr_gid);
> > >> + }
> > >> + socket_mode_str = getenv("WAYLAND_SERVER_MODE");
> > >> + if (socket_mode_str != NULL) {
> > >> + if (sscanf(socket_mode_str, "%o", &socket_mode) > 0)
> > >> + chmod(s->addr.sun_path, socket_mode);
> > >> + }
> > >> +
> > >>   s->source = wl_event_loop_add_fd(display->loop, s->fd,
> > >>   WL_EVENT_READABLE,
> > >>   socket_data, display);
> > >
> > > Hi,
> > >
> > > I'm not sure what to think of these two patches. Are environment
> > > variables WAYLAND_CLIENT_DIR, WAYLAND_SERVER_DIR, and the above really
> > > the right API for this?
> > >
> > > Your system compositor has to be specially written to be a system
> > > compositor and open multiple listening sockets anyway, so I think this
> > > is the wrong way for the server side stuff. For the server side, I
> > > don't think we should have anything of this controlled via environment
> > > variables but new API functions. What those API functions should be is
> > > a good question.
> > >
> > > I believe you don't need any additions in libwayland-server, in fact.
> > > You have very special requirements in how and where to create the
> > > sockets, so you can do that all yourself, up to the point where you have
> > > a socket fd listening for new connections. Then you can use
> > > wl_event_loop_add_fd() (if you even use the libwayland-server event
> > > loops stuff) to monitor the fd. When a client connects, you get a
> > > callback, call accept(), and pass the new fd to wl_client_create().
> > >
> > > Hmm, wait, why am I assuming multiple listening sockets... What is your
> > > architecture here, exactly?
> > >
> > > I have very hard time deciding if we should allow the environment to
> > > overwrite the server and client assumptions on where the socket is. It
> > > would be easier for me to accept new API functions that operate with
> > > absolute paths or existing fds explicitly, but those of course require
> > > both servers and clients to be written to use them.
> > >
> > > Comments, anyone?

Ah ah, sorry, I missed the more recent thread, where similar concerns
were already raised. I'll continue there, if there is any reason to.


Thanks,
pq
On 19.11.2014 12:56, Pekka Paalanen wrote:
> I have very hard time deciding if we should allow the environment to
> overwrite the server and client assumptions on where the socket is. It
> would be easier for me to accept new API functions that operate with
> absolute paths or existing fds explicitly, but those of course require
> both servers and clients to be written to use them.

A bit tricky part in current Weston is case where you use 
wayland-backend. In this case Weston is client to another Weston and in 
addition server providing a socket to it's client. In this setup the 
server is sort of proxy between lower level server and the client.

Since both instances solely use XDG_RUNTIME_DIR, the wayland-backend 
client weston is trying to connect to a socket there are create a new 
socket with same name in the same place...

The change is intended to allow a way to tell this second weston where 
to look for the client socket and where to place the server socket. 
Usually these two are not the same place...
On Tue, 25 Nov 2014 09:02:22 +0200
Jussi Laako <jussi.laako@linux.intel.com> wrote:

> On 19.11.2014 12:56, Pekka Paalanen wrote:
> > I have very hard time deciding if we should allow the environment to
> > overwrite the server and client assumptions on where the socket is. It
> > would be easier for me to accept new API functions that operate with
> > absolute paths or existing fds explicitly, but those of course require
> > both servers and clients to be written to use them.
> 
> A bit tricky part in current Weston is case where you use 
> wayland-backend. In this case Weston is client to another Weston and in 
> addition server providing a socket to it's client. In this setup the 
> server is sort of proxy between lower level server and the client.
> 
> Since both instances solely use XDG_RUNTIME_DIR, the wayland-backend 
> client weston is trying to connect to a socket there are create a new 
> socket with same name in the same place...
> 
> The change is intended to allow a way to tell this second weston where 
> to look for the client socket and where to place the server socket. 
> Usually these two are not the same place...

We don't have that problem anymore upstream. Now, if a socket file is
already taken, Weston just tries the next one until it finds a free
name, starts listening there, and exports WAYLAND_DISPLAY to
its own clients accordingly.

Before that was fixed, you could already use a command line argument to
use a socket with a different name.

However, all upstream cases will use XDG_RUNTIME_DIR, which probably is
not appropriate for your use case, depending on how you actually start
things.


Thanks,
pq
On 25.11.2014 15:01, Pekka Paalanen wrote:
> However, all upstream cases will use XDG_RUNTIME_DIR, which probably is
> not appropriate for your use case, depending on how you actually start
> things.

That's why I thought providing an alternative could be useful. It still 
supports XDG_RUNTIME_DIR too, but using the wayland specific variables 
allows override.