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

[Uri Lublin]

On 05/13/2012 02:40 PM, Alon Levy wrote:
> From: Yonit Halperin<yhalperi at 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.