[PATCH weston 1/2] compositor: fix comments about weston_compositor::surface_list

Tue May 17 09:17:45 UTC 2016

On Mon, 16 May 2016 16:29:18 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:

> On 10.05.2016 16:10, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > a7af70436b7dccfacd736626d6719b3e751fd985 converted the surface list into
> > a view list.
> > 
> > It looks like weston_surface::output's comment about surface list does
> > not apply to view list. Still, many places assume weston_surface::output
> > is not NULL when processing "visible" surfaces, e.g. those reachable via
> > the view list.
> > 
> > The comment on weston_view::output is updated, but I could not figure
> > out the actual relationship between that and being on the view list.
> > 
> > weston_view::link is documented to be in weston_compositor::view_list,
> > and weston_compositor::view_list is documented to contain weston_views.
> >   
> 
> It is always nice to see someone documenting the code. I've been banging
> my head lately to understand the codebase, and the currently present
> comments really help there.
> 
> With the issue noted below fixed, this patch is:
> 
> Reviewed-by: Armin Krezović <krezovic.armin at gmail.com>
> 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  src/compositor.h | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/compositor.h b/src/compositor.h
> > index a95f05d..7851000 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -754,7 +754,7 @@ struct weston_compositor {
> >  	struct wl_list output_list;
> >  	struct wl_list seat_list;
> >  	struct wl_list layer_list;
> > -	struct wl_list view_list;
> > +	struct wl_list view_list;	/* struct weston_view::link */
> >  	struct wl_list plane_list;
> >  	struct wl_list key_binding_list;
> >  	struct wl_list modifier_binding_list;
> > @@ -890,7 +890,7 @@ struct weston_view {
> >  	struct wl_list surface_link;
> >  	struct wl_signal destroy_signal;
> >  
> > -	struct wl_list link;
> > +	struct wl_list link;             /* weston_compositor::view_list */
> >  	struct weston_layer_entry layer_link; /* part of geometry */
> >  	struct weston_plane *plane;
> >  
> > @@ -951,7 +951,7 @@ struct weston_view {
> >  	/*
> >  	 * Which output to vsync this surface to.
> >  	 * Used to determine, whether to send or queue frame events.
> > -	 * Must be NULL, if 'link' is not in weston_compositor::surface_list.
> > +	 * Must be NULL, if 'link' is not in weston_compositor::view_list.  
> 
> I couldn't find if anything checks for this, and view->output can only be null
> if it got NULL from an output list. So I think the comment is redundant.

Btw. you cannot get a NULL from an output list, unless you first
explicitly check that the list is empty and bail out. Code that simply
does wl_container_of(output_list.next, output, link) or similar will
silently get a bad pointer and cause havoc later. The 'next' of an
empty list points to the list head itself, it's not NULL. And list head
is almost always not a valid list member.

Cool. I'm re-checking just to be sure. Marking weston_view::link as
deprecated, the compiler points me to the following cases:

weston_view_unmap():
	Bails out via weston_view_is_mapped() if view->output == NULL.
	Otherwise removes *and* initializes 'link'. So this is a
	potential issue, but the init hints that 'link' is intended to
	be always removable, which would void the issue.

weston_view_create() unconditionally inits 'link', and
weston_view_destroy() unconditionally removes 'link', so these confirm
that 'link' is always removable.

view_list_add_subsurface_view():
	Bails out if surface->output is NULL. (Not view->output!)
	Otherwise it does an unconditional wl_list_insert(view->link),
	which means the list 'link' was previously in, is now corrupt
	because it did not remove first.

view_list_add():
	Also does an unconditional wl_list_insert(view->link),
	which means the list 'link' was previously in, is now corrupt.

However, the corruption created there does not hurt, because those
functions are called from weston_compositor_build_view_list(), which
does wl_list_init() on the view_list, leaving all member of the list
orphaned and with corrupted 'link' to begin with.

But, there may be a danger there. If some views are not put back in the
view list during that weston_compositor_build_view_list() call, they
will be left with a corrupted 'link'. If you then remove 'link', it may
corrupt others. This path could use more investigation, and maybe we
should just fix it to keep all 'link' members always valid anyway by
doing the proper removes. (You can also remove a list head, it means
the elements remain in a headless list you cannot safely traverse, but
remove will work just fine.)

weston_plane_release():
	Traverses the view_list to reset plane pointers to NULL since
	the plane is being destroyed... looks like it might miss any
	views not in view_list though, which might cause problems if
	such view taken into use again.

weston_output_destroy():
	Calls weston_view_assign_output() for all views in view_list
	who overlapped the output being destroyed. Presumably shell
	plugin should be repositioning view before this to avoid the
	output becoming NULL... but the destroy signals are only
	emitted afterwards. How does that work?

That was the compositor.c uses of weston_view::link. Renderers and
assign_planes() implementations also use the view_list, but I don't
think they care what the weston_view::output is.

IOW, did not find a connection between link and output there.

I think I'll tweak the comments in these two patches a bit, and maybe
push then.

> >  	 */
> >  	struct weston_output *output;
> >  
> > @@ -1021,7 +1021,6 @@ struct weston_surface {
> >  	/*
> >  	 * Which output to vsync this surface to.
> >  	 * Used to determine, whether to send or queue frame events.
> > -	 * Must be NULL, if 'link' is not in weston_compositor::surface_list.
> >  	 */
> >  	struct weston_output *output;
> >  
> >   
> 

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160517/7b19cd95/attachment-0001.sig>