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

[Frediano Ziglio]

Make possible to safely call display_channel_drawable_unref straight
forward to get_drawable call.

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)
 {
-- 
2.4.3