[PATCH weston 1/4] Handle requests using outputs that have been unplugged
Jason Ekstrand
jason at jlekstrand.net
Mon May 19 13:04:20 PDT 2014
Neil,
Looks good except for the two comments below
--Jason
On Mon, May 19, 2014 at 12:23 PM, Neil Roberts <neil at linux.intel.com> wrote:
> Previously when an output was unplugged the clients' resources for it
> were left around and if they were used in a request it could cause
> Weston to access invalid memory. Now when an output is unplugged the
> resources for it are marked as zombies. This is done by setting a
> dummy dispatcher and setting the user data to NULL. The dispatcher
> ignores all requests except for one which is marked as a destructor.
> When the destructor is called the zombie resource will be destroyed.
> The opcode for the destructor request is stashed into one of the
> pointers in the resource's linked-list node so that it can be compared
> against all of the opcodes later. The wl_output interface doesn't
> currently have any requests nor a destructor but the intention is that
> this mechanism could also be used later for more complicated
> interfaces.
>
> Any requests that take a wl_output as an argument have also been
> patched to take into account that the output can now be a zombie.
> These are handled on a case-by-case basis but in general if the
> argument is allowed to be NULL then zombie resources will be treated
> as such. Otherwise the request is generally completely ignored.
>
> The screenshooter interface is a special case because that has a
> callback so we can't really just ignore the request. Instead the
> buffer for the screenshot is cleared and the callback is immediately
> invoked.
>
> The code for zombifying a resource is based on a snippet by Jason
> Esktrand.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=78415
> ---
> desktop-shell/input-panel.c | 6 +++++
> desktop-shell/shell.c | 38 ++++++++++++++++++++++-------
> fullscreen-shell/fullscreen-shell.c | 12 ++++++++++
> src/compositor.c | 36 ++++++++++++++++++++++++++++
> src/compositor.h | 3 +++
> src/screenshooter.c | 48
> +++++++++++++++++++++++++++++++++----
> 6 files changed, 130 insertions(+), 13 deletions(-)
>
> diff --git a/desktop-shell/input-panel.c b/desktop-shell/input-panel.c
> index 7623f6c..df365fb 100644
> --- a/desktop-shell/input-panel.c
> +++ b/desktop-shell/input-panel.c
> @@ -252,6 +252,12 @@ input_panel_surface_set_toplevel(struct wl_client
> *client,
> struct input_panel_surface *input_panel_surface =
> wl_resource_get_user_data(resource);
> struct desktop_shell *shell = input_panel_surface->shell;
> + struct weston_output *output;
> +
> + output = wl_resource_get_user_data(output_resource);
> + /* Ignore the request if the output has become a zombie */
> + if (output == NULL)
> + return;
>
> wl_list_insert(&shell->input_panel.surfaces,
> &input_panel_surface->link);
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index dd0b2f9..97c5d11 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -2431,10 +2431,12 @@ shell_surface_set_fullscreen(struct wl_client
> *client,
> struct shell_surface *shsurf = wl_resource_get_user_data(resource);
> struct weston_output *output;
>
> - if (output_resource)
> + if (output_resource) {
> output = wl_resource_get_user_data(output_resource);
> - else
> + /* Output can be NULL here if the resource is a zombie */
> + } else {
> output = NULL;
> + }
>
> shell_surface_set_parent(shsurf, NULL);
>
> @@ -2566,10 +2568,12 @@ shell_surface_set_maximized(struct wl_client
> *client,
> shsurf->type = SHELL_SURFACE_TOPLEVEL;
> shell_surface_set_parent(shsurf, NULL);
>
> - if (output_resource)
> + if (output_resource) {
> output = wl_resource_get_user_data(output_resource);
> - else
> + /* output can be NULL here if the resource is zombified */
> + } else {
> output = NULL;
> + }
>
> shell_surface_set_output(shsurf, output);
>
> @@ -3487,10 +3491,13 @@ xdg_surface_set_fullscreen(struct wl_client
> *client,
> shsurf->state_requested = true;
> shsurf->requested_state.fullscreen = true;
>
> - if (output_resource)
> + if (output_resource) {
> output = wl_resource_get_user_data(output_resource);
> - else
> + /* output can still be NULL here if the resource has
> + * become a zombie */
> + } else {
> output = NULL;
> + }
>
> shell_surface_set_output(shsurf, output);
> shsurf->fullscreen_output = shsurf->output;
> @@ -3919,6 +3926,10 @@ desktop_shell_set_background(struct wl_client
> *client,
> return;
> }
>
> + /* Skip the request if the output has become a zombie */
> + if (wl_resource_get_user_data(output_resource) == NULL)
> + return;
> +
> wl_list_for_each_safe(view, next, &surface->views, surface_link)
> weston_view_destroy(view);
> view = weston_view_create(surface);
> @@ -3953,6 +3964,7 @@ desktop_shell_set_panel(struct wl_client *client,
> struct desktop_shell *shell = wl_resource_get_user_data(resource);
> struct weston_surface *surface =
> wl_resource_get_user_data(surface_resource);
> + struct weston_output *output;
> struct weston_view *view, *next;
>
> if (surface->configure) {
> @@ -3962,14 +3974,20 @@ desktop_shell_set_panel(struct wl_client *client,
> return;
> }
>
> + output = wl_resource_get_user_data(output_resource);
> +
> + /* Skip the request if the output has become a zombie */
> + if (output == NULL)
> + return;
> +
> wl_list_for_each_safe(view, next, &surface->views, surface_link)
> weston_view_destroy(view);
> view = weston_view_create(surface);
>
> surface->configure = panel_configure;
> surface->configure_private = shell;
> - surface->output = wl_resource_get_user_data(output_resource);
> - view->output = surface->output;
> + surface->output = output;
> + view->output = output;
> desktop_shell_send_configure(resource, 0,
> surface_resource,
> surface->output->width,
> @@ -5362,6 +5380,10 @@ screensaver_set_surface(struct wl_client *client,
> struct weston_output *output =
> wl_resource_get_user_data(output_resource);
> struct weston_view *view, *next;
>
> + /* Skip the request if the output has become a zombie */
> + if (output == NULL)
> + return;
> +
> /* Make sure we only have one view */
> wl_list_for_each_safe(view, next, &surface->views, surface_link)
> weston_view_destroy(view);
> diff --git a/fullscreen-shell/fullscreen-shell.c
> b/fullscreen-shell/fullscreen-shell.c
> index a8dad8e..c3bc517 100644
> --- a/fullscreen-shell/fullscreen-shell.c
> +++ b/fullscreen-shell/fullscreen-shell.c
> @@ -673,6 +673,13 @@ fullscreen_shell_present_surface(struct wl_client
> *client,
>
> if (output_res) {
> output = wl_resource_get_user_data(output_res);
> + /* output can still be NULL here if the resource has
> + * become a zombie */
> + } else {
> + output = NULL;
> + }
>
Yeah, these are the wrong semantics. If they specify an output and it
turns out to be a zombie, we should do nothing. A null output in
wl_fullscreen_shell.present_surface means "put it somewhere". In the case
of weston's implementation, it goes on all outputs. We don't want a
surface to accidentally end up everywhere when it was intended for a
particular output.
> +
> + if (output) {
> fsout = fs_output_for_output(output);
> fs_output_set_surface(fsout, surface, method, 0, 0);
> } else {
> @@ -712,6 +719,11 @@ fullscreen_shell_present_surface_for_mode(struct
> wl_client *client,
> struct fs_output *fsout;
>
> output = wl_resource_get_user_data(output_res);
> +
> + /* Skip the request if the output has become a zombie */
> + if (output == NULL)
> + return;
> +
> fsout = fs_output_for_output(output);
>
> if (surface_res == NULL) {
> diff --git a/src/compositor.c b/src/compositor.c
> index 574db2d..ea0c9b4 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -3107,9 +3107,42 @@ weston_compositor_remove_output(struct
> weston_compositor *compositor,
> }
> }
>
> +static int
> +zombie_dispatcher(const void *data, void *resource, uint32_t opcode,
> + const struct wl_message *msg, union wl_argument *args)
> +{
> + struct wl_list *link = wl_resource_get_link(resource);
> + uint32_t destructor = (uintptr_t) link->next;
> +
> + if (opcode == destructor)
> + wl_resource_destroy(resource);
> +
> + return 0;
> +}
> +
> +WL_EXPORT void
> +weston_resource_zombify(struct wl_resource *res, uint32_t destructor)
> +{
> + struct wl_list *link;
> +
> + /* Set a dummy dispatcher so that all requests will be
> + * ignored. The NULL user_data is used to mark that this is a
> + * zombie */
> + wl_resource_set_dispatcher(res, zombie_dispatcher,
> + NULL /* implementation */,
> + NULL /* user data */,
> + NULL /* destroy */);
> + /* Stash the destructor opcode in one of the pointers of the
> + * list node so we can compare it later in the dispatcher */
> + link = wl_resource_get_link(res);
> + link->next = (struct wl_list *) (uintptr_t) destructor;
>
This is a bit ugly. In my original snippit, I pulled a similar stunt and
stashed the opcode in the implementation implementation pointer. Is there
some particular reason why you opted for the internal wl_list link
instead? I guess it doesn't really matter that much, but the
implementation is for storring dispatcher-specific stuff (such as an
opcode). Putting it in the internal wl_list link prevents someone from
keeping a list of zombie resources (no idea why they'd want to though).
> +}
> +
> WL_EXPORT void
> weston_output_destroy(struct weston_output *output)
> {
> + struct wl_resource *resource, *tmp;
> +
> output->destroying = 1;
>
> weston_compositor_remove_output(output->compositor, output);
> @@ -3124,6 +3157,9 @@ weston_output_destroy(struct weston_output *output)
> output->compositor->output_id_pool &= ~(1 << output->id);
>
> wl_global_destroy(output->global);
> +
> + wl_resource_for_each_safe(resource, tmp, &output->resource_list)
> + weston_resource_zombify(resource, ~0);
> }
>
> static void
> diff --git a/src/compositor.h b/src/compositor.h
> index 057f8be..6d9800a 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -1416,6 +1416,9 @@ weston_transformed_region(int width, int height,
> void *
> weston_load_module(const char *name, const char *entrypoint);
>
> +void
> +weston_resource_zombify(struct wl_resource *res, uint32_t destructor);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/src/screenshooter.c b/src/screenshooter.c
> index 369e920..574fde1 100644
> --- a/src/screenshooter.c
> +++ b/src/screenshooter.c
> @@ -213,6 +213,33 @@ weston_screenshooter_shoot(struct weston_output
> *output,
> }
>
> static void
> +shoot_zombie_output(struct weston_buffer *buffer,
> + weston_screenshooter_done_func_t done,
> + void *data)
> +{
> + struct wl_shm_buffer *shm_buffer;
> + int32_t height, stride;
> + void *pixels;
> +
> + shm_buffer = wl_shm_buffer_get(buffer->resource);
> +
> + if (shm_buffer == NULL) {
> + done(data, WESTON_SCREENSHOOTER_BAD_BUFFER);
> + return;
> + }
> +
> + height = wl_shm_buffer_get_height(shm_buffer);
> + stride = wl_shm_buffer_get_stride(shm_buffer);
> + pixels = wl_shm_buffer_get_data(shm_buffer);
> +
> + wl_shm_buffer_begin_access(shm_buffer);
> + memset(pixels, 0, height * stride);
> + wl_shm_buffer_end_access(shm_buffer);
> +
> + done(data, WESTON_SCREENSHOOTER_SUCCESS);
> +}
> +
> +static void
> screenshooter_done(void *data, enum weston_screenshooter_outcome outcome)
> {
> struct wl_resource *resource = data;
> @@ -235,17 +262,28 @@ screenshooter_shoot(struct wl_client *client,
> struct wl_resource *output_resource,
> struct wl_resource *buffer_resource)
> {
> - struct weston_output *output =
> - wl_resource_get_user_data(output_resource);
> - struct weston_buffer *buffer =
> - weston_buffer_from_resource(buffer_resource);
> + struct weston_output *output;
> + struct weston_buffer *buffer;
> +
> + output = wl_resource_get_user_data(output_resource);
> + buffer = weston_buffer_from_resource(buffer_resource);
>
> if (buffer == NULL) {
> wl_resource_post_no_memory(resource);
> return;
> }
>
> - weston_screenshooter_shoot(output, buffer, screenshooter_done,
> resource);
> + /* If the output has become a zombie then we obviously can't
> + * take a screenshot. We don't want to report an error because
> + * that would likely make the client abort and this is an
> + * unavoidable occurence. Instead we can just clear the
> + * buffer. This is probably reasonable because that is what
> + * the output will actually if it is unplugged */
> + if (output == NULL)
> + shoot_zombie_output(buffer, screenshooter_done, resource);
> + else
> + weston_screenshooter_shoot(output, buffer,
> + screenshooter_done, resource);
> }
>
> struct screenshooter_interface screenshooter_implementation = {
> --
> 1.9.0
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140519/2f958e3b/attachment-0001.html>
More information about the wayland-devel
mailing list