[Spice-devel] [PATCH 05/22] display: make get_drawable symmetric to display_channel_drawable_unref

[Jonathon Jongsma]

On Wed, 2015-12-02 at 12:57 -0500, Frediano Ziglio wrote:
> > 
> > Frediano,
> > 
> > On Wed, Dec 2, 2015 at 5:19 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> > > Make possible to safely call display_channel_drawable_unref straight
> > > forward to get_drawable call.
> > 
> > Sorry, but I didn't get what's the problem we have with the current code.
> > Would you mind to give me a more detailed an explanation?
> > 
> 
> It's a problem of definition and dependency.
> 
> display_channel_drawable_try_new is supposed to return an uninitialized
> pointer (or NULL on failure) to a Drawable structure.
> 
> (display_channel_)get_drawable is supposed to return an initialized
> pointer (or NULL) to a Drawable structure.
> 
> (display_channel_)add_drawable is supposed to add the Drawable to the
> list/tree of drawing to draw.
> 
> Now, with these definitions after get_drawable the Drawable state (if
> pointer is not NULL) should be consistent and we should be able to call
> display_channel_drawable_unref.
> 
> In the current code this was not true as for instance surface_id was
> copied to Drawable but the reference counter of the surface was not
> incremented leading possible unref call to decrement the counter and
> free the surface. This can happen if any call between get_drawable and
> unref does not increment the reference in a consistent way. This
> basically means that every present or future code in the path
> between get_drawable and unref have to know this unconsistency
> and handle it.
> This is a maintaining problem as people need to know these details
> when editing existing code (actually this is display_channel_add_drawable
> code).
> This basically create a dependency between get_drawable and
> add_drawable.
> 
> Previous patch did also another mistake. It incremented the references
> while validating the structure leading to wrong counters updates in case
> some part was not valid while other was. This can happen for instance
> if guest pass a valid surface_id and an invalid surface_deps.
> 
> Actually I think these consideration should be better in some code
> documentation as discussed previously. I'll add these comments as part
> of the code.

Yes, or at a minimum, this (good) description should be in the commit log. 



> 
> Frediano
> 
> 
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  server/display-channel.c | 20 +++++++++-----------
> > >  1 file changed, 9 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > index 809673b..0750de8 100644
> > > --- a/server/display-channel.c
> > > +++ b/server/display-channel.c
> > > @@ -1117,7 +1117,16 @@ Drawable
> > > *display_channel_get_drawable(DisplayChannel *display, uint8_t effect,
> > >      drawable->red_drawable = red_drawable_ref(red_drawable);
> > > 
> > >      drawable->surface_id = red_drawable->surface_id;
> > > +    display->surfaces[drawable->surface_id].refs++;
> > > +
> > >      memcpy(drawable->surface_deps, red_drawable->surface_deps,
> > >      sizeof(drawable->surface_deps));
> > > +    /*
> > > +        surface->refs is affected by a drawable (that is
> > > +        dependent on the surface) as long as the drawable is alive.
> > > +        However, surface->depend_on_me is affected by a drawable only
> > > +        as long as it is in the current tree (hasn't been rendered yet).
> > > +    */
> > > +    red_inc_surfaces_drawable_dependencies(display, drawable);
> > > 
> > >      return drawable;
> > >  }
> > > @@ -1128,9 +1137,6 @@ void display_channel_add_drawable(DisplayChannel
> > > *display, Drawable *drawable)
> > >      RedDrawable *red_drawable = drawable->red_drawable;
> > > 
> > >      red_drawable->mm_time = reds_get_mm_time();
> > > -    surface_id = drawable->surface_id;
> > > -
> > > -    display->surfaces[surface_id].refs++;
> > > 
> > >      region_add(&drawable->tree_item.base.rgn, &red_drawable->bbox);
> > > 
> > > @@ -1143,14 +1149,6 @@ void display_channel_add_drawable(DisplayChannel
> > > *display, Drawable *drawable)
> > >          region_destroy(&rgn);
> > >      }
> > > 
> > > -    /*
> > > -        surface->refs is affected by a drawable (that is
> > > -        dependent on the surface) as long as the drawable is alive.
> > > -        However, surface->depend_on_me is affected by a drawable only
> > > -        as long as it is in the current tree (hasn't been rendered yet).
> > > -    */
> > > -    red_inc_surfaces_drawable_dependencies(display, drawable);
> > > -
> > >      if (region_is_empty(&drawable->tree_item.base.rgn)) {
> > >          return;
> > >      }
> > > --
> > > 2.4.3
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel