Re: [PATCH 3.1/12] Store QXLInstance in CursorItem

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2015-10-30 at 03:24 -0400, Frediano Ziglio wrote:
> > 
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > ---
> >  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 6cc2b93..2d7afc6 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);
> 
> This catch a memory corruption, should abort the program,
> I suggest spice_error, spice_critical or spice_assert.

It can suggest memory corruption, yes. But I'm not sure that it should
be fatal. This is exactly the same behavior as g_object_ref(), so I
don't see any good reason to make this warning fatal when the exact
same problem with a gobject will not be fatal. 

> 
> >  
> > -    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)
> > +        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;
> >  }
> >  
> 
> Are these tests worth doing or can we assume that a NULL pointer is
> expected (like
> unref(NULL) and ref(NULL) do nothing) ? This would make
> cursor_set_item like
> 
> static void cursor_set_item(CursorChannel *cursor, CursorItem *item)
> {
>     cursor_item_unref(cursor->item);
> 
>     cursor->item = cursor_item_ref(item);
> }


I'd prefer to stay closer to the semantics of g_object_ref/unref(),
which warn if the argument is not a valid GObject.

> 
> 
> On naming, sometimes is cursor_item sometimes item. I would prefer
> cursor_item.
> 
> >  static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void
> > *data, int
> >  num)
> > @@ -155,7 +158,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);
> >  }
> >  
> > @@ -411,7 +414,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:
> > @@ -439,7 +446,8 @@ void cursor_channel_process_cmd(CursorChannel
> > *cursor,
> > RedCursorCmd *cursor_cmd,
> >          red_channel_pipes_new_add(&cursor->common.base,
> >          new_cursor_pipe_item,
> >                                    (void*)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 2c19859..8b49df7 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(CommonChannel
> > *common,
> >                                                 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_ */
> 
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]