[PATCH wayland 2/3] server: Split out code to open sockets for a specific display name

[Jasper St. Pierre]

On Wed, May 7, 2014 at 9:24 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:

> Comments below,
> --Jason Ekstrand
>
> On Wed, May 7, 2014 at 9:25 AM, Jasper St. Pierre <jstpierre at mecheye.net>wrote:
>
>> We'll use this to autodetect a good socket to open on.
>> ---
>>  src/wayland-server.c | 40 +++++++++++++++++++++++++---------------
>>  1 file changed, 25 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/wayland-server.c b/src/wayland-server.c
>> index d0fd280..6bc8dc3 100644
>> --- a/src/wayland-server.c
>> +++ b/src/wayland-server.c
>> @@ -1039,11 +1039,9 @@ get_socket_lock(struct wl_socket *socket)
>>         return fd_lock;
>>  }
>>
>> -WL_EXPORT int
>> -wl_display_add_socket(struct wl_display *display, const char *name)
>> +static int
>> +open_socket_for_display_name(struct wl_socket *s, const char *name)
>>
>
> Perhaps, this should be named differently.  How about wl_socket_try_lock.
> The problem is that it doesn't really open the socket (assuming I put this
> and the previous patch together correctly in my head)
>

This function fills in the sun_path given the DISPLAY_NAME and tries the
lock. Perhaps these should be separate: split out the sun_path stuff to
setup_socket_for_display_name. Maybe we could combine them as
try_socket_lock_for_display_name.

 {
>> -       struct wl_socket *s;
>> -       socklen_t size;
>>         int name_size;
>>         const char *runtime_dir;
>>
>> @@ -1057,15 +1055,6 @@ wl_display_add_socket(struct wl_display *display,
>> const char *name)
>>                 return -1;
>>         }
>>
>> -       s = malloc(sizeof *s);
>> -       if (s == NULL)
>> -               return -1;
>> -
>> -       if (name == NULL)
>> -               name = getenv("WAYLAND_DISPLAY");
>> -       if (name == NULL)
>> -               name = "wayland-0";
>> -
>>         memset(&s->addr, 0, sizeof s->addr);
>>         s->addr.sun_family = AF_LOCAL;
>>         name_size = snprintf(s->addr.sun_path, sizeof s->addr.sun_path,
>> @@ -1075,7 +1064,6 @@ wl_display_add_socket(struct wl_display *display,
>> const char *name)
>>         if (name_size > (int)sizeof s->addr.sun_path) {
>>                 wl_log("error: socket path \"%s/%s\" plus null terminator"
>>                        " exceeds 108 bytes\n", runtime_dir, name);
>> -               free(s);
>>                 /* to prevent programs reporting
>>                  * "failed to add socket: Success" */
>>                 errno = ENAMETOOLONG;
>>
>
> Why are we filling addr structure here and doing nothing with it? Perhaps
> filling the addr structure should stay in wl_display_add_socket?
>

We are doing something with it. get_socket_lock uses it to find the correct
lock name to take.


> @@ -1084,6 +1072,28 @@ wl_display_add_socket(struct wl_display *display,
>> const char *name)
>>
>>         s->fd_lock = get_socket_lock(s);
>>         if (s->fd_lock < 0) {
>> +               return -1;
>> +       }
>>
>
> This is right before opening the socket, so it returns with an invalid
> value in s->fd.
>

Hm? s->fd is opened below taking the lock in wl_display_add_socket. s->fd
is untouched from its initial value, which should always be 0.


> +
>> +       return 0;
>> +}
>> +
>> +WL_EXPORT int
>> +wl_display_add_socket(struct wl_display *display, const char *name)
>> +{
>> +       struct wl_socket *s;
>> +       socklen_t size;
>> +
>> +       s = malloc(sizeof *s);
>> +       if (s == NULL)
>> +               return -1;
>> +
>> +       if (name == NULL)
>> +               name = getenv("WAYLAND_DISPLAY");
>> +       if (name == NULL)
>> +               name = "wayland-0";
>> +
>> +       if (open_socket_for_display_name(s, name) < 0) {
>>                 free(s);
>>                 return -1;
>>         }
>> @@ -1095,7 +1105,7 @@ wl_display_add_socket(struct wl_display *display,
>> const char *name)
>>                 return -1;
>>         }
>>
>> -       size = offsetof (struct sockaddr_un, sun_path) + name_size;
>> +       size = offsetof (struct sockaddr_un, sun_path) +
>> strlen(s->addr.sun_path);
>>         if (bind(s->fd, (struct sockaddr *) &s->addr, size) < 0) {
>>                 wl_log("bind() failed with error: %m\n");
>>                 close(s->fd);
>> --
>> 1.9.0
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>
>


-- 
  Jasper
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140508/e98b4685/attachment.html>