Neil, Looks good except for the two comments below --Jason
On Mon, May 19, 2014 at 12:23 PM, Neil Roberts <n...@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@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