[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