Re: [PATCH wayland] server: Don't expose wl_display as a global
On Thu, 21 Aug 2014 09:05:48 -0700
Jason Ekstrand ja...@jlekstrand.net wrote:
On Thu, Aug 21, 2014 at 4:00 AM, Pekka Paalanen ppaala...@gmail.com
wrote:
On Thu, 7 Aug 2014 09:55:49 -0400
Jasper St. Pierre jstpie...@mecheye.net wrote:
The idea here was that once upon a time, clients could rebind
wl_display to a higher version, so we offered the ability to
rebind it here. However, this is particularly broken. The
existing bind implementation actually still hardcodes version
numbers, and it leaks previous resources, overwriting the
existing one.
The newly bound resource *also* won't have any listeners attached
by the client, meaning that the error and delete_id events won't
get delivered correctly. Unless the client poked into libwayland
internals, it also can't possibly set up these handlers correctly
either, so the client will sustain errors and leak all deleted
globals.
Since this never worked correctly in the first place, we can feel
safe removing it.
---
src/wayland-server.c | 28
1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/src/wayland-server.c b/src/wayland-server.c
index 3c162d4..dc3f502 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -380,9 +380,8 @@ wl_client_get_display(struct wl_client
*client) return client-display;
}
-static void
-bind_display(struct wl_client *client,
- void *data, uint32_t version, uint32_t id);
+static int
+bind_display(struct wl_client *client, struct wl_display
*display);
/** Create a client for the given file descriptor
*
@@ -440,9 +439,7 @@ wl_client_create(struct wl_display *display,
int fd) goto err_map;
wl_signal_init(client-destroy_signal);
- bind_display(client, display, 1, 1);
-
- if (!client-display_resource)
+ if (bind_display(client, display) 0)
goto err_map;
wl_list_insert(display-client_list.prev, client-link);
@@ -772,22 +769,20 @@ destroy_client_display_resource(struct
wl_resource
*resource)
resource-client-display_resource = NULL;
}
-static void
-bind_display(struct wl_client *client,
- void *data, uint32_t version, uint32_t id)
+static int
+bind_display(struct wl_client *client, struct wl_display
*display) {
- struct wl_display *display = data;
-
client-display_resource =
- wl_resource_create(client, wl_display_interface,
1, id);
+ wl_resource_create(client, wl_display_interface,
1, 1); if (client-display_resource == NULL) {
wl_client_post_no_memory(client);
- return;
+ return -1;
}
wl_resource_set_implementation(client-display_resource,
display_interface, display,
destroy_client_display_resource);
+ return 0;
}
/** Create Wayland display object.
@@ -831,13 +826,6 @@ wl_display_create(void)
wl_array_init(display-additional_shm_formats);
- if (!wl_global_create(display, wl_display_interface, 1,
- display, bind_display)) {
- wl_event_loop_destroy(display-loop);
- free(display);
- return NULL;
- }
-
return display;
}
I asked krh about this in irc, and he eventually agreed that
exposing wl_display as a global is not so useful.
I reviewed the patch, and the alleged problems the current code has,
and I agree they are real.
If we ever end up needing wl_display as a global, we can easily add
it back. Since the implementation was indeed broken, no client
should ever try to bind to wl_display global with version 1.
Therefore, pushed with ack from Jason and R-b from me.
In fact, I think this warrants this patch as a condidate for the
stable branch.
Yeah, it probably counts as a bug-fix. It does change behaviour
though, so I'm not sure I really want to call it a stable thing.
Unless, of course, re-binding wl_display could cause a crash; in
which case, push it to 1.5 --Jason
Let's see if there is any fallout during the alpha period, and think
about the stable branch then.
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] server: Don't expose wl_display as a global
On Thu, 7 Aug 2014 09:55:49 -0400
Jasper St. Pierre jstpie...@mecheye.net wrote:
The idea here was that once upon a time, clients could rebind wl_display
to a higher version, so we offered the ability to rebind it
here. However, this is particularly broken. The existing bind
implementation actually still hardcodes version numbers, and it leaks
previous resources, overwriting the existing one.
The newly bound resource *also* won't have any listeners attached by the
client, meaning that the error and delete_id events won't get delivered
correctly. Unless the client poked into libwayland internals, it also
can't possibly set up these handlers correctly either, so the client
will sustain errors and leak all deleted globals.
Since this never worked correctly in the first place, we can feel safe
removing it.
---
src/wayland-server.c | 28
1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/src/wayland-server.c b/src/wayland-server.c
index 3c162d4..dc3f502 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -380,9 +380,8 @@ wl_client_get_display(struct wl_client *client)
return client-display;
}
-static void
-bind_display(struct wl_client *client,
- void *data, uint32_t version, uint32_t id);
+static int
+bind_display(struct wl_client *client, struct wl_display *display);
/** Create a client for the given file descriptor
*
@@ -440,9 +439,7 @@ wl_client_create(struct wl_display *display, int fd)
goto err_map;
wl_signal_init(client-destroy_signal);
- bind_display(client, display, 1, 1);
-
- if (!client-display_resource)
+ if (bind_display(client, display) 0)
goto err_map;
wl_list_insert(display-client_list.prev, client-link);
@@ -772,22 +769,20 @@ destroy_client_display_resource(struct wl_resource
*resource)
resource-client-display_resource = NULL;
}
-static void
-bind_display(struct wl_client *client,
- void *data, uint32_t version, uint32_t id)
+static int
+bind_display(struct wl_client *client, struct wl_display *display)
{
- struct wl_display *display = data;
-
client-display_resource =
- wl_resource_create(client, wl_display_interface, 1, id);
+ wl_resource_create(client, wl_display_interface, 1, 1);
if (client-display_resource == NULL) {
wl_client_post_no_memory(client);
- return;
+ return -1;
}
wl_resource_set_implementation(client-display_resource,
display_interface, display,
destroy_client_display_resource);
+ return 0;
}
/** Create Wayland display object.
@@ -831,13 +826,6 @@ wl_display_create(void)
wl_array_init(display-additional_shm_formats);
- if (!wl_global_create(display, wl_display_interface, 1,
- display, bind_display)) {
- wl_event_loop_destroy(display-loop);
- free(display);
- return NULL;
- }
-
return display;
}
I asked krh about this in irc, and he eventually agreed that exposing
wl_display as a global is not so useful.
I reviewed the patch, and the alleged problems the current code has,
and I agree they are real.
If we ever end up needing wl_display as a global, we can easily add it
back. Since the implementation was indeed broken, no client should ever
try to bind to wl_display global with version 1.
Therefore, pushed with ack from Jason and R-b from me.
In fact, I think this warrants this patch as a condidate for the stable
branch.
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] server: Don't expose wl_display as a global
On Thu, Aug 21, 2014 at 4:00 AM, Pekka Paalanen ppaala...@gmail.com wrote:
On Thu, 7 Aug 2014 09:55:49 -0400
Jasper St. Pierre jstpie...@mecheye.net wrote:
The idea here was that once upon a time, clients could rebind wl_display
to a higher version, so we offered the ability to rebind it
here. However, this is particularly broken. The existing bind
implementation actually still hardcodes version numbers, and it leaks
previous resources, overwriting the existing one.
The newly bound resource *also* won't have any listeners attached by the
client, meaning that the error and delete_id events won't get delivered
correctly. Unless the client poked into libwayland internals, it also
can't possibly set up these handlers correctly either, so the client
will sustain errors and leak all deleted globals.
Since this never worked correctly in the first place, we can feel safe
removing it.
---
src/wayland-server.c | 28
1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/src/wayland-server.c b/src/wayland-server.c
index 3c162d4..dc3f502 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -380,9 +380,8 @@ wl_client_get_display(struct wl_client *client)
return client-display;
}
-static void
-bind_display(struct wl_client *client,
- void *data, uint32_t version, uint32_t id);
+static int
+bind_display(struct wl_client *client, struct wl_display *display);
/** Create a client for the given file descriptor
*
@@ -440,9 +439,7 @@ wl_client_create(struct wl_display *display, int fd)
goto err_map;
wl_signal_init(client-destroy_signal);
- bind_display(client, display, 1, 1);
-
- if (!client-display_resource)
+ if (bind_display(client, display) 0)
goto err_map;
wl_list_insert(display-client_list.prev, client-link);
@@ -772,22 +769,20 @@ destroy_client_display_resource(struct wl_resource
*resource)
resource-client-display_resource = NULL;
}
-static void
-bind_display(struct wl_client *client,
- void *data, uint32_t version, uint32_t id)
+static int
+bind_display(struct wl_client *client, struct wl_display *display)
{
- struct wl_display *display = data;
-
client-display_resource =
- wl_resource_create(client, wl_display_interface, 1, id);
+ wl_resource_create(client, wl_display_interface, 1, 1);
if (client-display_resource == NULL) {
wl_client_post_no_memory(client);
- return;
+ return -1;
}
wl_resource_set_implementation(client-display_resource,
display_interface, display,
destroy_client_display_resource);
+ return 0;
}
/** Create Wayland display object.
@@ -831,13 +826,6 @@ wl_display_create(void)
wl_array_init(display-additional_shm_formats);
- if (!wl_global_create(display, wl_display_interface, 1,
- display, bind_display)) {
- wl_event_loop_destroy(display-loop);
- free(display);
- return NULL;
- }
-
return display;
}
I asked krh about this in irc, and he eventually agreed that exposing
wl_display as a global is not so useful.
I reviewed the patch, and the alleged problems the current code has,
and I agree they are real.
If we ever end up needing wl_display as a global, we can easily add it
back. Since the implementation was indeed broken, no client should ever
try to bind to wl_display global with version 1.
Therefore, pushed with ack from Jason and R-b from me.
In fact, I think this warrants this patch as a condidate for the stable
branch.
Yeah, it probably counts as a bug-fix. It does change behaviour though, so
I'm not sure I really want to call it a stable thing. Unless, of course,
re-binding wl_display could cause a crash; in which case, push it to 1.5
--Jason
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] server: Don't expose wl_display as a global
On Fri, 8 Aug 2014 14:55:43 -0700
Jason Ekstrand ja...@jlekstrand.net wrote:
I think this is ok. To my knowledge, no one is re-binding wl_display or
even relying on that global being exposed.
I cannot imagine any reason to advertize wl_display as a global
either. Personally I have always thought the version of wl_display
is immutable, and if the re-binding really is as broken as you say,
good riddance.
But, I would still like to have Kristian's comment here, so we're
not forgetting some detail on why it was there in the first place.
Thanks,
pq
On Thu, Aug 7, 2014 at 6:55 AM, Jasper St. Pierre jstpie...@mecheye.net
wrote:
The idea here was that once upon a time, clients could rebind wl_display
to a higher version, so we offered the ability to rebind it
here. However, this is particularly broken. The existing bind
implementation actually still hardcodes version numbers, and it leaks
previous resources, overwriting the existing one.
The newly bound resource *also* won't have any listeners attached by the
client, meaning that the error and delete_id events won't get delivered
correctly. Unless the client poked into libwayland internals, it also
can't possibly set up these handlers correctly either, so the client
will sustain errors and leak all deleted globals.
Since this never worked correctly in the first place, we can feel safe
removing it.
---
src/wayland-server.c | 28
1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/src/wayland-server.c b/src/wayland-server.c
index 3c162d4..dc3f502 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -380,9 +380,8 @@ wl_client_get_display(struct wl_client *client)
return client-display;
}
-static void
-bind_display(struct wl_client *client,
-void *data, uint32_t version, uint32_t id);
+static int
+bind_display(struct wl_client *client, struct wl_display *display);
/** Create a client for the given file descriptor
*
@@ -440,9 +439,7 @@ wl_client_create(struct wl_display *display, int fd)
goto err_map;
wl_signal_init(client-destroy_signal);
- bind_display(client, display, 1, 1);
-
- if (!client-display_resource)
+ if (bind_display(client, display) 0)
goto err_map;
wl_list_insert(display-client_list.prev, client-link);
@@ -772,22 +769,20 @@ destroy_client_display_resource(struct wl_resource
*resource)
resource-client-display_resource = NULL;
}
-static void
-bind_display(struct wl_client *client,
-void *data, uint32_t version, uint32_t id)
+static int
+bind_display(struct wl_client *client, struct wl_display *display)
{
- struct wl_display *display = data;
-
client-display_resource =
- wl_resource_create(client, wl_display_interface, 1, id);
+ wl_resource_create(client, wl_display_interface, 1, 1);
if (client-display_resource == NULL) {
wl_client_post_no_memory(client);
- return;
+ return -1;
}
wl_resource_set_implementation(client-display_resource,
display_interface, display,
destroy_client_display_resource);
+ return 0;
}
/** Create Wayland display object.
@@ -831,13 +826,6 @@ wl_display_create(void)
wl_array_init(display-additional_shm_formats);
- if (!wl_global_create(display, wl_display_interface, 1,
- display, bind_display)) {
- wl_event_loop_destroy(display-loop);
- free(display);
- return NULL;
- }
-
return display;
}
--
2.0.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] server: Don't expose wl_display as a global
I think this is ok. To my knowledge, no one is re-binding wl_display or
even relying on that global being exposed.
--Jason
On Thu, Aug 7, 2014 at 6:55 AM, Jasper St. Pierre jstpie...@mecheye.net
wrote:
The idea here was that once upon a time, clients could rebind wl_display
to a higher version, so we offered the ability to rebind it
here. However, this is particularly broken. The existing bind
implementation actually still hardcodes version numbers, and it leaks
previous resources, overwriting the existing one.
The newly bound resource *also* won't have any listeners attached by the
client, meaning that the error and delete_id events won't get delivered
correctly. Unless the client poked into libwayland internals, it also
can't possibly set up these handlers correctly either, so the client
will sustain errors and leak all deleted globals.
Since this never worked correctly in the first place, we can feel safe
removing it.
---
src/wayland-server.c | 28
1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/src/wayland-server.c b/src/wayland-server.c
index 3c162d4..dc3f502 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -380,9 +380,8 @@ wl_client_get_display(struct wl_client *client)
return client-display;
}
-static void
-bind_display(struct wl_client *client,
-void *data, uint32_t version, uint32_t id);
+static int
+bind_display(struct wl_client *client, struct wl_display *display);
/** Create a client for the given file descriptor
*
@@ -440,9 +439,7 @@ wl_client_create(struct wl_display *display, int fd)
goto err_map;
wl_signal_init(client-destroy_signal);
- bind_display(client, display, 1, 1);
-
- if (!client-display_resource)
+ if (bind_display(client, display) 0)
goto err_map;
wl_list_insert(display-client_list.prev, client-link);
@@ -772,22 +769,20 @@ destroy_client_display_resource(struct wl_resource
*resource)
resource-client-display_resource = NULL;
}
-static void
-bind_display(struct wl_client *client,
-void *data, uint32_t version, uint32_t id)
+static int
+bind_display(struct wl_client *client, struct wl_display *display)
{
- struct wl_display *display = data;
-
client-display_resource =
- wl_resource_create(client, wl_display_interface, 1, id);
+ wl_resource_create(client, wl_display_interface, 1, 1);
if (client-display_resource == NULL) {
wl_client_post_no_memory(client);
- return;
+ return -1;
}
wl_resource_set_implementation(client-display_resource,
display_interface, display,
destroy_client_display_resource);
+ return 0;
}
/** Create Wayland display object.
@@ -831,13 +826,6 @@ wl_display_create(void)
wl_array_init(display-additional_shm_formats);
- if (!wl_global_create(display, wl_display_interface, 1,
- display, bind_display)) {
- wl_event_loop_destroy(display-loop);
- free(display);
- return NULL;
- }
-
return display;
}
--
2.0.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
