[Spice-devel] [PATCH spice-server 6/6] Move display field from Drawable to _Drawable (pool object)

[Jonathon Jongsma]

hrm.  I guess I'm not strongly opposed, but I don't see this as a big
improvement. I dislike the way this 'pool' is implemented in general,
and I don't think this makes it much cleaner. 

I think I disagree with the statement that "the pointer is only used to
maintain the pool". It is also used to notify the channel that the
drawable has been freed so that it can clean up dependent objects, etc.
Those things would need to be done whether the object was allocated
from a pool or allocated dynamically, I think.

It also means adding more SPICE_CONTAINER_OF() stuff which I don't
really like.

Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>



On Fri, 2017-12-08 at 15:55 +0000, Frediano Ziglio wrote:
> This pointer is used only to maintain the pool.
> There is no reason to expose this to the other objects.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/display-channel-private.h | 1 +
>  server/display-channel.c         | 7 ++-----
>  server/display-channel.h         | 1 -
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/server/display-channel-private.h b/server/display-
> channel-private.h
> index 617ce30d..ff3dde30 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -69,6 +69,7 @@ struct _Drawable {
>          Drawable drawable;
>          _Drawable *next;
>      } u;
> +    DisplayChannel *display;
>  };
>  
>  struct DisplayChannelPrivate
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 899df42c..9ac9c5c5 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1594,6 +1594,7 @@ static Drawable*
> drawable_try_new(DisplayChannel *display)
>      if (!display->priv->free_drawables)
>          return NULL;
>  
> +    display->priv->free_drawables->display = display;
>      drawable = &display->priv->free_drawables->u.drawable;
>      display->priv->free_drawables = display->priv->free_drawables-
> >u.next;
>      display->priv->drawable_count++;
> @@ -1633,10 +1634,6 @@ static Drawable
> *display_channel_drawable_try_new(DisplayChannel *display,
>      }
>  
>      bzero(drawable, sizeof(Drawable));
> -    /* Pointer to the display from which the drawable is
> allocated.  This
> -     * pointer is safe to be retained as DisplayChannel lifespan is
> bigger than
> -     * all drawables.  */
> -    drawable->display = display;
>      drawable->refs = 1;
>      drawable->creation_time = drawable->first_frame_time =
> spice_get_monotonic_time_ns();
>      ring_item_init(&drawable->list_link);
> @@ -1688,7 +1685,7 @@ static void
> drawable_unref_surface_deps(DisplayChannel *display, Drawable *drawa
>  
>  void drawable_unref(Drawable *drawable)
>  {
> -    DisplayChannel *display = drawable->display;
> +    DisplayChannel *display = SPICE_CONTAINEROF(drawable, _Drawable,
> u.drawable)->display;
>  
>      if (--drawable->refs != 0)
>          return;
> diff --git a/server/display-channel.h b/server/display-channel.h
> index e26d2ba1..04c4f3d5 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -104,7 +104,6 @@ struct Drawable {
>      int surface_deps[3];
>  
>      uint32_t process_commands_generation;
> -    DisplayChannel *display;
>  };
>  
>  DisplayChannel*            display_channel_new                      
>  (RedsState *reds,