[PATCH weston 04/15] libweston: let add/remove_output handle the lists

[Pekka Paalanen]

On Fri, 7 Apr 2017 20:58:45 +0200
Armin Krezović <krezovic.armin at gmail.com> wrote:

> On 04.04.2017 12:58, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > A weston_output available to the compositor should always be either in
> > the pending or the live outputs list. Let weston_compositor_add_output()
> > and weston_compositor_remove_output() handle the moves between the
> > lists.
> > 
> > This way weston_output_enable() does not need to remove and
> > oops-it-failed-add-it-back. weston_output_disable() does not need to
> > manually re-add the output back to the pending list.
> > 
> > To make everything nicely symmetric and fix any unbalancing caused by
> > this:
> > - weston_output_destroy() explicitly wl_list_remove()s
> > - weston_compositor_add_pending_output() first removes then inserts, as
> > we have the assumption that the link is always valid, even if empty.
> > 
> > Update the documentations, too.
> >   
> 
> Hi again.
> 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  libweston/compositor.c | 28 +++++++++++++++-------------
> >  1 file changed, 15 insertions(+), 13 deletions(-)
> > 
> > diff --git a/libweston/compositor.c b/libweston/compositor.c
> > index 60cbae0..09a3db2 100644
> > --- a/libweston/compositor.c
> > +++ b/libweston/compositor.c
> > @@ -4537,6 +4541,8 @@ weston_output_enable_undo(struct weston_output *output)
> >   *
> >   * - wl_output protocol objects referencing this weston_output
> >   * are made inert.
> > + *
> > + * - The output is put back in the pending outputs list.
> >   */
> >  static void
> >  weston_compositor_remove_output(struct weston_output *output)
> > @@ -4555,7 +4561,6 @@ weston_compositor_remove_output(struct weston_output *output)
> >  	weston_presentation_feedback_discard_list(&output->feedback_list);
> >  
> >  	weston_compositor_reflow_outputs(compositor, output, output->width);
> > -	wl_list_remove(&output->link);
> >    
> 
> When I looked more closely at this part when I applied your patches, I found a
> potential problem with moving this down below. The original code first removed
> the output from the enabled output list, then emitted the signals.
> 
> Signal listeners might iterate through output list before this function "returns",
> ie, to pick a new output for a view, and could potentially pick this output again.
> 
> Even worse, wl_list_empty(&c->output_list) will return false even if this is the
> last output going away, throwing previous work about "all outputs gone at runtime"
> down the drain.

Hi Armin,

yes, a very good catch. I'll fix that, probably by moving both remove
and insert to where the remove used to be. In fact, I'm thinking if
that list manipulation should be the very first thing that function
does.

I've also fixed the "live outputs" wording.


Thanks,
pq

> >  	wl_signal_emit(&compositor->output_destroyed_signal, output);
> >  	wl_signal_emit(&output->destroy_signal, output);
> > @@ -4563,6 +4568,9 @@ weston_compositor_remove_output(struct weston_output *output)
> >  	wl_resource_for_each(resource, &output->resource_list) {
> >  		wl_resource_set_destructor(resource, NULL);
> >  	}
> > +
> > +	wl_list_remove(&output->link);
> > +	wl_list_insert(compositor->pending_output_list.prev, &output->link);
> >  }
> >    
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170724/d2e7fe89/attachment.sig>