[Spice-devel] [PATCH 02/10] Store QXLInstance in CursorItem

Jonathon Jongsma jjongsma at redhat.com
Mon Nov 2 12:18:52 PST 2015


On Mon, 2015-11-02 at 11:00 -0500, Frediano Ziglio wrote:
> > 
> > On Mon, Nov 2, 2015 at 10:55 AM, Frediano Ziglio <
> > fziglio at redhat.com> wrote:
> > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > 
> > > Doing so allows us to remove the extra QXLInstance parameter from
> > > cursor_item_unref() and makes the code a bit cleaner.
> > > 
> > > Also add cursor_item_ref().
> > > 
> > > Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> > > ---
> > >  server/cursor-channel.c | 70
> > >  +++++++++++++++++++++++++++----------------------
> > >  server/cursor-channel.h |  9 ++++---
> > >  2 files changed, 44 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > > index eef0121..d145f86 100644
> > > --- a/server/cursor-channel.c
> > > +++ b/server/cursor-channel.c
> > > @@ -25,55 +25,58 @@
> > >  #include "cache_item.tmpl.c"
> > >  #undef CLIENT_CURSOR_CACHE
> > > 
> > > -static inline CursorItem *alloc_cursor_item(void)
> > > +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd,
> > > uint32_t
> > > group_id)
> > >  {
> > >      CursorItem *cursor_item;
> > > 
> > > +    spice_return_val_if_fail(cmd != NULL, NULL);
> > > +
> > >      cursor_item = g_slice_new0(CursorItem);
> > > +    cursor_item->qxl = qxl;
> > >      cursor_item->refs = 1;
> > > +    cursor_item->group_id = group_id;
> > > +    cursor_item->red_cursor = cmd;
> > > 
> > >      return cursor_item;
> > >  }
> > > 
> > > -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t
> > > group_id)
> > > +CursorItem *cursor_item_ref(CursorItem *item)
> > >  {
> > > -    CursorItem *cursor_item;
> > > -
> > > -    spice_return_val_if_fail(cmd != NULL, NULL);
> > > -    cursor_item = alloc_cursor_item();
> > > +    spice_return_val_if_fail(item != NULL, NULL);
> > > +    spice_return_val_if_fail(item->refs > 0, NULL);
> > > 
> > > -    cursor_item->group_id = group_id;
> > > -    cursor_item->red_cursor = cmd;
> > > +    item->refs++;
> > > 
> > > -    return cursor_item;
> > > +    return item;
> > >  }
> > > 
> > > -void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor)
> > > +void cursor_item_unref(CursorItem *item)
> > >  {
> > > -    if (!--cursor->refs) {
> > > -        QXLReleaseInfoExt release_info_ext;
> > > -        RedCursorCmd *cursor_cmd;
> > > -
> > > -        cursor_cmd = cursor->red_cursor;
> > > -        release_info_ext.group_id = cursor->group_id;
> > > -        release_info_ext.info = cursor_cmd->release_info;
> > > -        qxl->st->qif->release_resource(qxl, release_info_ext);
> > > -        red_put_cursor_cmd(cursor_cmd);
> > > -        free(cursor_cmd);
> > > -
> > > -        g_slice_free(CursorItem, cursor);
> > > -    }
> > > +    QXLReleaseInfoExt release_info_ext;
> > > +    RedCursorCmd *cursor_cmd;
> > > +
> > > +    spice_return_if_fail(item != NULL);
> > > +
> > > +    if (--item->refs)
> > 
> > Just a nitpick, I would prefer to have a explicit comparison here:
> > if
> > (--items->refs > 0) ...
> > 
> 
> Why not 
> 
>   if (--items->refs != 0) ??
> 
> I mean, at the end the results should be the same if no errors
> managing
> the counters are present. Are you suggesting to change the test to
> avoid
> multiple free (hoping memory is still untouched after the first one)
> ?
> 
> I start suspecting that sending the patch when there are pending
> issue to
> be discussed is not really a good idea.


Hmm, what do you mean by this? What pending issues need to be
discussed?



> 
> > > +        return;
> > > +
> > > +    cursor_cmd = item->red_cursor;
> > > +    release_info_ext.group_id = item->group_id;
> > > +    release_info_ext.info = cursor_cmd->release_info;
> > > +    item->qxl->st->qif->release_resource(item->qxl,
> > > release_info_ext);
> > > +    red_put_cursor_cmd(cursor_cmd);
> > > +    free(cursor_cmd);
> > > +
> > > +    g_slice_free(CursorItem, item);
> > > +
> > >  }
> > > 
> > >  static void cursor_set_item(CursorChannel *cursor, CursorItem
> > > *item)
> > >  {
> > >      if (cursor->item)
> > > -        cursor_item_unref(red_worker_get_qxl(cursor
> > > ->common.worker),
> > > cursor->item);
> > > -
> > > -    if (item)
> > > -        item->refs++;
> > > +        cursor_item_unref(cursor->item);
> > > 
> > > -    cursor->item = item;
> > > +    cursor->item = item ? cursor_item_ref(item) : NULL;
> > >  }
> > > 
> > >  static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc,
> > > void *data,
> > >  int num)
> > > @@ -157,7 +160,7 @@ static void
> > > put_cursor_pipe_item(CursorChannelClient
> > > *ccc, CursorPipeItem *pipe_
> > > 
> > >      spice_assert(!pipe_item_is_linked(&pipe_item->base));
> > > 
> > > -    cursor_item_unref(red_worker_get_qxl(ccc->common.worker),
> > > pipe_item->cursor_item);
> > > +    cursor_item_unref(pipe_item->cursor_item);
> > >      free(pipe_item);
> > >  }
> > > 
> > > @@ -404,7 +407,11 @@ void
> > > cursor_channel_process_cmd(CursorChannel *cursor,
> > > RedCursorCmd *cursor_cmd,
> > >      CursorItem *cursor_item;
> > >      int cursor_show = FALSE;
> > > 
> > > -    cursor_item = cursor_item_new(cursor_cmd, group_id);
> > > +    spice_return_if_fail(cursor);
> > > +    spice_return_if_fail(cursor_cmd);
> > > +
> > > +    cursor_item =
> > > cursor_item_new(red_worker_get_qxl(cursor->common.worker),
> > > +                                  cursor_cmd, group_id);
> > > 
> > >      switch (cursor_cmd->type) {
> > >      case QXL_CURSOR_SET:
> > > @@ -434,7 +441,8 @@ void cursor_channel_process_cmd(CursorChannel
> > > *cursor,
> > > RedCursorCmd *cursor_cmd,
> > >          red_channel_pipes_new_add(&cursor->common.base,
> > >                                    new_cursor_pipe_item,
> > > cursor_item);
> > >      }
> > > -    cursor_item_unref(red_worker_get_qxl(cursor->common.worker),
> > > cursor_item);
> > > +
> > > +    cursor_item_unref(cursor_item);
> > >  }
> > > 
> > >  void cursor_channel_reset(CursorChannel *cursor)
> > > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > > index 293cfc1..f20001c 100644
> > > --- a/server/cursor-channel.h
> > > +++ b/server/cursor-channel.h
> > > @@ -33,6 +33,7 @@
> > >  #define CURSOR_CACHE_HASH_KEY(id) ((id) &
> > > CURSOR_CACHE_HASH_MASK)
> > > 
> > >  typedef struct CursorItem {
> > > +    QXLInstance *qxl;
> > >      uint32_t group_id;
> > >      int refs;
> > >      RedCursorCmd *red_cursor;
> > > @@ -83,14 +84,14 @@ void                 cursor_channel_reset
> > > (CursorChannel *cursor);
> > >  void                 cursor_channel_process_cmd (CursorChannel
> > > *cursor,
> > >  RedCursorCmd *cursor_cmd,
> > >                                                   uint32_t
> > > group_id);
> > > 
> > > -CursorItem*          cursor_item_new            (RedCursorCmd
> > > *cmd,
> > > uint32_t group_id);
> > > -void                 cursor_item_unref          (QXLInstance
> > > *qxl,
> > > CursorItem *cursor);
> > > -
> > > -
> > >  CursorChannelClient* cursor_channel_client_new(CursorChannel
> > > *cursor,
> > >                                                 RedClient
> > > *client,
> > >                                                 RedsStream
> > > *stream,
> > >                                                 int mig_target,
> > >                                                 uint32_t
> > > *common_caps, int
> > >                                                 num_common_caps,
> > >                                                 uint32_t *caps,
> > > int
> > >                                                 num_caps);
> > > 
> > > +CursorItem*          cursor_item_new            (QXLInstance
> > > *qxl,
> > > RedCursorCmd *cmd, uint32_t group_id);
> > > +CursorItem*          cursor_item_ref            (CursorItem
> > > *cursor);
> > > +void                 cursor_item_unref          (CursorItem
> > > *cursor);
> > > +
> > >  #endif /* CURSOR_CHANNEL_H_ */
> > > --
> > > 2.4.3
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> > ACK!
> > 
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel


More information about the Spice-devel mailing list