Re: [PATCH weston 1/4] Handle requests using outputs that have been unplugged
On Tue, 27 May 2014 10:47:44 +0300
Pekka Paalanen wrote:
> Hi,
>
> I see you posted a v2 of this patch, but I think my comments below are
> still valid.
>
>
> On Mon, 19 May 2014 18:23:45 +0100
> Neil Roberts wrote:
>
> > 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;
> > +}
>
> You are overwriting the destroy vfunc, how do we get the user data
> destroyed? Is the caller responsible for calling the destroy vfunc
> manually first?
>
> I think a better API would be to have wl_resource destroy function that
> turns the object inert. It would call the destroy vfunc automatically.
> Also documenting it as a "delayed" destructor would make the point,
> that as long as the compositor is concerned, the protocol object is
> gone, which also means you are free to abuse the list pointers (ugh).
>
> wl_resource_zombify() also has the limitation, that it cannot handle
> interfaces which have requests creating new protocol objects. This
> should be clearly documented, but it also makes me wonder if this is
> really worth it.
I mean, let's look at what this buys us.
By using this unobvious mechanism, the programmer can avoid writing the
three lines in each request handler:
if (!mydata)
return;
This mechanism does not handle any of the hard cases, like requests
that create new objects. If an interface gets extended and a request
creating new objects is added, suddenly all the zombification paths are
now broken.
This also does not help any other interfaces which have objects of this
interface as request arguments.
Am I missing something?
To me this seems closer to obfuscation than a clean-up, when compared to
the convention of setting userdata to NULL on inert objects, and just
dealing with it. Furthermore, you can handle all cases, including
requests creating new objects, with the same convention in a
future-proof way.
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/4] Handle requests using outputs that have been unplugged
Hi,
I see you posted a v2 of this patch, but I think my comments below are
still valid.
On Mon, 19 May 2014 18:23:45 +0100
Neil Roberts 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.
I think we should be using the term "inert".
"zombie" already means a protocol object id, that is still in the object
map waiting for confirmation that the other side has acknowledged its
destruction, but there is nothing existing for it anymore (the wl_proxy
has already been destroyed).
To me "zombie" refers to protocol objects that are already destroyed on
one side, and waiting for the other side to ack.
"Inert" OTOH means a protocol object that is not destroyed, it still
exists, but what it used to mean or refer to is no longer valid.
Protocol objects created from binding to globals are just one example.
Another example is a wl_sub_surface, whose wl_surface has been
destroyed.
Inert is also a term that is already being used in the protocol
specifications.
> 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;
> + }
It could be NULL also on purpose, so the comment might be misleading.
>
> 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;
> + }
It could be NULL also on purpose.
>
> shell_surface_set_output(shsurf, output);
>
> @@ -3487
Re: [PATCH weston 1/4] Handle requests using outputs that have been unplugged
Jason Ekstrand writes: > 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. The ~0 is meant to signify ‘there is no destructor’, ie, it's a fake opcode that will never match a request. The reason is that I put this patch first before the separate patch to handle the wl_output.release request which changes it to 0. Yes, it would probably be good to have a define for this but as it was only in one temporary patch I thought it wasn't worth adding. Maybe just reordering the patches would make it cleaner. Regards, - Neil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/4] Handle requests using outputs that have been unplugged
Jason Ekstrand writes: > 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. Ok yes, that makes sense. > 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? Well, the main reason was that I wanted to avoid having to add a wl_resource_get_implementation function, but I agree it would be cleaner to just add that as you say. Thanks for the review. - Neil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/4] Handle requests using outputs that have been unplugged
One more comment
On Mon, May 19, 2014 at 3:04 PM, Jason Ekstrand wrote:
> Neil,
> Looks good except for the two comments below
> --Jason
>
>
> On Mon, May 19, 2014 at 12:23 PM, Neil Roberts 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_
Re: [PATCH weston 1/4] Handle requests using outputs that have been unplugged
Neil,
Looks good except for the two comments below
--Jason
On Mon, May 19, 2014 at 12:23 PM, Neil Roberts 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)
> +
