[Spice-devel] [PATCH 02/15] display: make get_drawable symmetric to display_channel_drawable_unref

[Frediano Ziglio]

> 
> On Thu, 2015-12-03 at 16:26 +0000, Frediano Ziglio wrote:
> > Make possible to safely call display_channel_drawable_unref straight
> > forward to get_drawable call.
> 
> I would suggest changing this to "straight after calling get_drawable"
> 
> > 
> > Problem was function definitions 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.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/display-channel.c | 39 ++++++++++++++++++++++++++++-----------
> >  1 file changed, 28 insertions(+), 11 deletions(-)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 722ee86..007512e 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1091,6 +1091,13 @@ static int validate_drawable_bbox(DisplayChannel
> > *display, RedDrawable *drawable
> >          return TRUE;
> >  }
> >  
> > +/**
> > + * @brief Get a new Drawable
> > + *
> > + * The Drawable returned is fully initialized.
> > + *
> > + * @return initialized Drawable or NULL on failure
> > + */
> >  Drawable *display_channel_get_drawable(DisplayChannel *display, uint8_t
> > effect,
> >                                         RedDrawable *red_drawable, uint32_t
> > group_id,
> >                                         uint32_t
> >                                         process_commands_generation)
> > @@ -1098,6 +1105,9 @@ Drawable *display_channel_get_drawable(DisplayChannel
> > *display, uint8_t effect,
> >      Drawable *drawable;
> >      int x;
> >  
> > +    /* Validate all surface ids before updating counters
> > +     * to avoid invalid updates if we find an invalid id.
> > +     */
> >      if (!validate_drawable_bbox(display, red_drawable)) {
> >          return NULL;
> >      }
> > @@ -1117,20 +1127,30 @@ 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;
> >  }
> >  
> > +/**
> > + * Add a Drawable to the items to draw.
> > + * On failure the Drawable is not added.
> > + */
> >  void display_channel_add_drawable(DisplayChannel *display, Drawable
> > *drawable)
> >  {
> >      int success = FALSE, surface_id = drawable->surface_id;
> >      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 +1163,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;
> >      }
> > @@ -1348,6 +1360,11 @@ static void drawables_init(DisplayChannel *display)
> >      }
> >  }
> >  
> > +/**
> > + * Allocate a Drawable
> > + *
> > + * @return pointer to uninitialized Drawable or NULL on failure
> > + */
> >  Drawable *display_channel_drawable_try_new(DisplayChannel *display,
> >                                             int group_id, int
> > process_commands_generation)
> >  {
> 
> 
> Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
> 

Merged

Frediano