>
> 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@xxxxxxxxxx>
>
>
>
> 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@xxxxxxxxxx>
> > ---
> > 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,
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel