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

[Frediano Ziglio]

> 
> 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.

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
>