[PATCH weston v3] ivi-shell: bugfix, an ivi_surface is not removed from list of ivi_layer when the ivi_surface is removed from the compositor.
Ucan, Emre (ADITG/SW1)
eucan at de.adit-jv.com
Thu Aug 20 05:02:10 PDT 2015
Hi Pekka,
I will send a new patch after I fixed these issues. I used this commit message because it was used before by Tanibata-san.
If you want, I can send it also with another (shorter) commit message.
Best regards
Emre Ucan
Software Group I (ADITG/SW1)
> -----Original Message-----
> From: Pekka Paalanen [mailto:ppaalanen at gmail.com]
> Sent: Donnerstag, 20. August 2015 11:44
> To: Ucan, Emre (ADITG/SW1)
> Cc: wayland-devel at lists.freedesktop.org; Tanibata, Nobuhiko (ADITJ/SWG)
> Subject: Re: [PATCH weston v3] ivi-shell: bugfix, an ivi_surface is not
> removed from list of ivi_layer when the ivi_surface is removed from the
> compositor.
>
> On Wed, 19 Aug 2015 11:25:02 +0000
> "Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:
>
> > - Introduce the dirty parameter for triggering the render order change
> > - IVI_NOTIFICATION_REMOVE/ADD flags are set only at commit_layer_list.
> >
> > Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
>
> Hi Ucan-san and Tanibata-san,
>
> there are a couple of commit message issues one should pay attention to in
> the future.
>
> The email subject, a.k.a commit summary line, is too long. It should be limited
> to roughly around 60 characters or so. A good summary is unique,
> descriptive, and short. It seems you are doing the opposite:
> the summary is a long prose, while the message body has short bullet points.
>
> When you are sending v2, v3, etc. of a patch, it would be good to include a list
> of changes, see e.g.
> http://cgit.freedesktop.org/wayland/weston/commit/?id=23ab7159d200883
> cc0d21db8dc4efdd58e2d60a7
>
> Since we are now doing small patches to ivi-layout.c, I will be more strict on
> wl_list manipulation. There is a lot to clean up here.
>
> (Btw. I am a little confused here on the architecture. If the purpose of struct
> link_layer is to allow an ivi-surface to be on multiple ivi-layers, how can a
> direct list like struct ivi_layout_layer::order.surface_list work? Below it looks
> like struct ivi_layout_surface::order.link is what is stored in that list, but you
> cannot store the same link in multiple lists at the same time.)
>
> > ---
> > ivi-shell/ivi-layout-private.h | 1 +
> > ivi-shell/ivi-layout.c | 63 ++++++++++++++--------------------------
> > 2 files changed, 23 insertions(+), 41 deletions(-)
> >
> > diff --git a/ivi-shell/ivi-layout-private.h
> > b/ivi-shell/ivi-layout-private.h index 9c04c30..074d598 100644
> > --- a/ivi-shell/ivi-layout-private.h
> > +++ b/ivi-shell/ivi-layout-private.h
> > @@ -81,6 +81,7 @@ struct ivi_layout_layer {
> > } pending;
> >
> > struct {
> > + int dirty;
> > struct wl_list surface_list;
> > struct wl_list link;
> > } order;
> > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c index
> > d412069..bc8aead 100644
> > --- a/ivi-shell/ivi-layout.c
> > +++ b/ivi-shell/ivi-layout.c
> > @@ -809,53 +809,38 @@ commit_layer_list(struct ivi_layout *layout)
> >
> > ivilayer->prop = ivilayer->pending.prop;
> >
> > - if (!(ivilayer->event_mask &
> > - (IVI_NOTIFICATION_ADD | IVI_NOTIFICATION_REMOVE))
> ) {
> > + if (!ivilayer->order.dirty) {
> > continue;
> > }
> >
> > - if (ivilayer->event_mask & IVI_NOTIFICATION_REMOVE) {
> > - wl_list_for_each_safe(ivisurf, next,
> > - &ivilayer->order.surface_list, order.link) {
> > - remove_ordersurface_from_layer(ivisurf);
> > + wl_list_for_each_safe(ivisurf, next,
> > + &ivilayer->order.surface_list, order.link) {
> > + remove_ordersurface_from_layer(ivisurf);
> >
> > - if (!wl_list_empty(&ivisurf->order.link)) {
> > - wl_list_remove(&ivisurf->order.link);
> > - }
> > -
> > - wl_list_init(&ivisurf->order.link);
> > - ivisurf->event_mask |=
> IVI_NOTIFICATION_REMOVE;
> > + if (!wl_list_empty(&ivisurf->order.link)) {
> > + wl_list_remove(&ivisurf->order.link);
>
> While at it, let's fix these mis-patterns that give a false sense of safety.
>
> wl_list_empty() is not a valid way to check if a link can be wl_list_remove()'d.
>
> Checking wl_list_empty() on a link offers no information: if it returns true,
> wl_list_remove() is safe to do. If it returns false, you still do not know if
> wl_list_remove() is safe; the link could be part of a list, or the link could be
> "uninitialized" (e.g. just wl_list_remove()'d).
>
> You need some other information to know whether a link is safe to remove.
> It could be another variable, or it could be a design decision
> (invariant) that it must be always safe to remove.
>
> As the wl_list_empty() check is completely bogus here, you can just remove
> it and do the wl_list_remove() unconditionally. If it leads to a crash, the code
> was already broken before. I also do not think it can lead to a crash, because
> the only case when the check avoids calling
> wl_list_remove() is the only case according to wl_list_empty() that removing
> actually is safe.
>
> > }
> >
> > - wl_list_init(&ivilayer->order.surface_list);
> > + wl_list_init(&ivisurf->order.link);
> > + ivisurf->event_mask |=
> IVI_NOTIFICATION_REMOVE;
> > }
> >
> > - if (ivilayer->event_mask & IVI_NOTIFICATION_ADD) {
> > - wl_list_for_each_safe(ivisurf, next,
> > - &ivilayer->order.surface_list,
> order.link) {
> > - remove_ordersurface_from_layer(ivisurf);
> > -
> > - if (!wl_list_empty(&ivisurf->order.link)) {
> > - wl_list_remove(&ivisurf->order.link);
> > - }
> > + wl_list_init(&ivilayer->order.surface_list);
>
> This wl_list_init() is confusing. Either the wl_list_for_each_safe loop above
> removes all items from the list in which case the init is redundant, or if there
> are items still in the list then those items are left in a broken list. The list also
> cannot be "uninitialized" because we just iterated through it.
>
> If you want to ensure and document that the list really is empty at this point,
> do this:
> assert(wl_list_empty(&ivilayer->order.surface_list));
>
> >
> > + wl_list_for_each(ivisurf, &ivilayer->pending.surface_list,
> > + pending.link) {
> > + if (!wl_list_empty(&ivisurf->order.link)){
> > + wl_list_remove(&ivisurf->order.link);
>
> Here's another bogus empty check.
>
> > wl_list_init(&ivisurf->order.link);
>
> Init before insert is redundant. wl_list_insert() does not read the link before
> overwriting it, so it is ok if the link is "uninitialized".
>
> > }
> >
> > - wl_list_init(&ivilayer->order.surface_list);
> > - wl_list_for_each(ivisurf, &ivilayer-
> >pending.surface_list,
> > - pending.link) {
> > - if (!wl_list_empty(&ivisurf->order.link)) {
> > - wl_list_remove(&ivisurf->order.link);
> > - wl_list_init(&ivisurf->order.link);
> > - }
> > -
> > - wl_list_insert(&ivilayer->order.surface_list,
> > - &ivisurf->order.link);
> > - add_ordersurface_to_layer(ivisurf, ivilayer);
> > - ivisurf->event_mask |=
> IVI_NOTIFICATION_ADD;
> > - }
> > + wl_list_insert(&ivilayer->order.surface_list,
> > + &ivisurf->order.link);
> > + add_ordersurface_to_layer(ivisurf, ivilayer);
> > + ivisurf->event_mask |= IVI_NOTIFICATION_ADD;
> > }
> > +
> > + ivilayer->order.dirty = 0;
> > }
> > }
> >
> > @@ -997,8 +982,6 @@ clear_surface_pending_list(struct ivi_layout_layer
> > *ivilayer)
> >
> > wl_list_init(&surface_link->pending.link);
> > }
> > -
> > - ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE;
> > }
> >
> > static void
> > @@ -1015,8 +998,6 @@ clear_surface_order_list(struct ivi_layout_layer
> > *ivilayer)
> >
> > wl_list_init(&surface_link->order.link);
> > }
> > -
> > - ivilayer->event_mask |= IVI_NOTIFICATION_REMOVE;
> > }
> >
> > static void
> > @@ -2102,7 +2083,7 @@ ivi_layout_layer_set_render_order(struct
> ivi_layout_layer *ivilayer,
> > }
> > }
> >
> > - ivilayer->event_mask |= IVI_NOTIFICATION_ADD;
> > + ivilayer->order.dirty = 1;
> >
> > return IVI_SUCCEEDED;
> > }
> > @@ -2526,7 +2507,7 @@ ivi_layout_layer_add_surface(struct
> ivi_layout_layer *ivilayer,
> > }
> > }
> >
> > - ivilayer->event_mask |= IVI_NOTIFICATION_ADD;
> > + ivilayer->order.dirty = 1;
> >
> > return IVI_SUCCEEDED;
> > }
> > @@ -2554,7 +2535,7 @@ ivi_layout_layer_remove_surface(struct
> ivi_layout_layer *ivilayer,
> > }
> > }
> >
> > - remsurf->event_mask |= IVI_NOTIFICATION_REMOVE;
> > + ivilayer->order.dirty = 1;
> > }
> >
> > static int32_t
>
> Other than the mentioned above, the patch looks good to me. Using 'dirty'
> instead of those flags sounds sensible, though I'm not too familiar with the
> architecture here.
>
>
> Thanks,
> pq
More information about the wayland-devel
mailing list