One more comment
On Mon, May 19, 2014 at 3:04 PM, Jason Ekstrand <ja...@jlekstrand.net>wrote: > 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); >> > 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@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