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

[Frediano Ziglio]

> 
> 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.

Yes, you are right. Don't know why I didn't pay attention to this.
Nack by myself too.

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

This is not a bit issue actually. Usually even free is implemented
in a way similar to this, it suppose there's an header before the
pointer.

> 
> 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,
>