[Spice-devel] server/red_worker: don't release self_bitmap unless refcount is 0

Submitted by Alon Levy on May 13, 2012, 11:40 a.m.

Details

Message ID 1336909212-14104-1-git-send-email-alevy@redhat.com
State Superseded
Commit 6935bd3d604288a64f491e4226f3d31aafdbf81e
Headers show

Not browsing as part of any series.

Commit Message

Alon Levy May 13, 2012, 11:40 a.m.
From: Yonit Halperin <yhalperi@redhat.com>

RHBZ: 808936
---
 server/red_worker.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/red_worker.c b/server/red_worker.c
index 473d0d6..60f30d3 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -1695,13 +1695,12 @@  static inline void put_red_drawable(RedWorker *worker, RedDrawable *drawable, ui
 {
     QXLReleaseInfoExt release_info_ext;
 
-    if (self_bitmap) {
-        red_put_image(self_bitmap);
-    }
     if (--drawable->refs) {
         return;
     }
-
+    if (self_bitmap) {
+        red_put_image(self_bitmap);
+    }
     worker->red_drawable_count--;
     release_info_ext.group_id = group_id;
     release_info_ext.info = drawable->release_info;

Comments

On 05/13/2012 02:40 PM, Alon Levy wrote:
> From: Yonit Halperin<yhalperi@redhat.com>
>
> RHBZ: 808936
> ---
>   server/red_worker.c |    7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 473d0d6..60f30d3 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -1695,13 +1695,12 @@ static inline void put_red_drawable(RedWorker *worker, RedDrawable *drawable, ui
>   {
>       QXLReleaseInfoExt release_info_ext;
>
> -    if (self_bitmap) {
> -        red_put_image(self_bitmap);
> -    }
>       if (--drawable->refs) {
>           return;
>       }
> -
> +    if (self_bitmap) {
> +        red_put_image(self_bitmap);
> +    }
>       worker->red_drawable_count--;
>       release_info_ext.group_id = group_id;
>       release_info_ext.info = drawable->release_info;


That patch itself looks good, assuming "self_bitmap" must be alive when 
"drawable" is alive.

A few comments/questions:
put_red_drawable is called from 3 places, one of which calls it with a 
NULL self_bitmap.
Is it possible this call will be the last (and self_bitmap would leak) ?

Also it is not clear to me if self_bitmap is updated together with 
drawable->refs.
For example in get_drawable self_bitmap is NULL, till we get an image from
the driver (in red_process_commands -> red_process_drawable ->  
red_handle_self_bitmap).
I think it makes sense to get self_bitmap inside RedDrawable (can be in 
a different patch).
It would clean the code a bit.

(So much text for such a simple patch)

Regards,
     Uri.