[PATCH weston 1/4] Handle requests using outputs that have been unplugged

[Pekka Paalanen]

On Tue, 27 May 2014 10:47:44 +0300
Pekka Paalanen <ppaalanen at 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 <neil at 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