[PATCH weston 1/4] Handle requests using outputs that have been unplugged
Jason Ekstrand
jason at jlekstrand.net
Mon May 19 13:08:17 PDT 2014
One more comment
On Mon, May 19, 2014 at 3:04 PM, Jason Ekstrand <jason at jlekstrand.net>wrote:
> 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);
>>
>
The destructor passed in here should be 0, why is it ~0? Also, it might be
a good idea to throw it in a #define or enum for clarity.
> }
>>
>> 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/8889101e/attachment-0001.html>
More information about the wayland-devel
mailing list