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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel