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

[Armin Krezović]

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.

>  	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: signature.asc
Type: application/pgp-signature
Size: 870 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170407/c93eb7a7/attachment.sig>