On Tue, 27 May 2014 10:47:44 +0300 Pekka Paalanen <ppaala...@gmail.com> 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 <n...@linux.intel.com> 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