Re: [PATCH v3] Add features to PipeItem class

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

 



On Thu, 2016-04-14 at 13:07 -0400, Frediano Ziglio wrote:
> > 
> > On Thu, 2016-04-14 at 10:54 +0100, Frediano Ziglio wrote:
> > > From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> > > 
> > > Add reference counting to PipeItem.
> > > A user-defined callback is called when the refcount drops to 0.
> > > 
> > > Reference counting is manually coded for several classes deriving from
> > > PipeItem, so this change will help to share this code, and allow to remove
> > > some ref/unref virtual functions in some interfaces when we can assume
> > > every instance derives from this base class.
> > > 
> > > Signed-off-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > > ---
> > >  server/Makefile.am     |  2 ++
> > >  server/red-channel.c   |  6 ------
> > >  server/red-channel.h   | 13 +-----------
> > >  server/red-pipe-item.c | 53
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  server/red-pipe-item.h | 57
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  5 files changed, 113 insertions(+), 18 deletions(-)
> > >  create mode 100644 server/red-pipe-item.c
> > >  create mode 100644 server/red-pipe-item.h
> > > 
> > > Changes from v2:
> > > - instead of adding another class extend to existing one.
> > >   Reasons:
> > >   - is not clear from the name the difference from PipeItem and
> > >     RedPipeItem;
> > >   - all derived classes does not use "Red" prefix;
> > >   - reference counting is use quite a lot (also take into
> > >     consideration channel_hold_pipe_item_proc and similar).
> > > - the memory required is not too much (on 64 bit structures
> > >   get increases by 8 bytes and there are some hundred of them
> > >   at a time).
> > > 
> > > 
> > > diff --git a/server/Makefile.am b/server/Makefile.am
> > > index a7a8d9f..a119c86 100644
> > > --- a/server/Makefile.am
> > > +++ b/server/Makefile.am
> > > @@ -112,6 +112,8 @@ libserver_la_SOURCES =				
> > > \
> > >  	display-channel.h			\
> > >  	cursor-channel.c			\
> > >  	cursor-channel.h			\
> > > +	red-pipe-item.c				\
> > > +	red-pipe-item.h				\
> > >  	reds.c					\
> > >  	reds.h					\
> > >  	reds-private.h				\
> > > diff --git a/server/red-channel.c b/server/red-channel.c
> > > index cfddea0..3e036c9 100644
> > > --- a/server/red-channel.c
> > > +++ b/server/red-channel.c
> > > @@ -1658,12 +1658,6 @@ void
> > > red_channel_client_set_message_serial(RedChannelClient *rcc, uint64_t
> > > seria
> > >      rcc->send_data.serial = serial;
> > >  }
> > >  
> > > -void pipe_item_init(PipeItem *item, int type)
> > > -{
> > > -    ring_item_init(&item->link);
> > > -    item->type = type;
> > > -}
> > > -
> > >  static inline gboolean client_pipe_add(RedChannelClient *rcc, PipeItem
> > >  *item,
> > > RingItem *pos)
> > >  {
> > >      spice_assert(rcc && item);
> > > diff --git a/server/red-channel.h b/server/red-channel.h
> > > index 94b09eb..3c762ff 100644
> > > --- a/server/red-channel.h
> > > +++ b/server/red-channel.h
> > > @@ -33,6 +33,7 @@
> > >  #include "demarshallers.h"
> > >  #include "reds-stream.h"
> > >  #include "stat.h"
> > > +#include "red-pipe-item.h"
> > >  
> > >  #define MAX_SEND_BUFS 1000
> > >  #define CLIENT_ACK_WINDOW 20
> > > @@ -147,16 +148,6 @@ enum {
> > >      PIPE_ITEM_TYPE_CHANNEL_BASE=101,
> > >  };
> > >  
> > > -typedef struct PipeItem {
> > > -    RingItem link;
> > > -    int type;
> > > -} PipeItem;
> > > -
> > > -static inline int pipe_item_is_linked(PipeItem *item)
> > > -{
> > > -    return ring_item_is_linked(&item->link);
> > > -}
> > > -
> > >  typedef uint8_t *(*channel_alloc_msg_recv_buf_proc)(RedChannelClient
> > > *channel,
> > >                                                      uint16_t type,
> > >                                                      uint32_t
> > > size);
> > >  typedef int (*channel_handle_parsed_proc)(RedChannelClient *rcc, uint32_t
> > > size, uint16_t type,
> > > @@ -473,8 +464,6 @@ int
> > > red_channel_client_get_roundtrip_ms(RedChannelClient
> > > *rcc);
> > >   */
> > >  void red_channel_client_start_connectivity_monitoring(RedChannelClient
> > >  *rcc,
> > > uint32_t timeout_ms);
> > >  
> > > -void pipe_item_init(PipeItem *item, int type);
> > > -
> > >  // TODO: add back the channel_pipe_add functionality - by adding
> > > reference
> > > counting
> > >  // to the PipeItem.
> > >  
> > > diff --git a/server/red-pipe-item.c b/server/red-pipe-item.c
> > > new file mode 100644
> > > index 0000000..d94d76c
> > > --- /dev/null
> > > +++ b/server/red-pipe-item.c
> > > @@ -0,0 +1,53 @@
> > > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > > +/*
> > > +   Copyright (C) 2015 Red Hat, Inc.
> > > +
> > > +   This library is free software; you can redistribute it and/or
> > > +   modify it under the terms of the GNU Lesser General Public
> > > +   License as published by the Free Software Foundation; either
> > > +   version 2.1 of the License, or (at your option) any later version.
> > > +
> > > +   This library is distributed in the hope that it will be useful,
> > > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > +   Lesser General Public License for more details.
> > > +
> > > +   You should have received a copy of the GNU Lesser General Public
> > > +   License along with this library; if not, see
> > > <http://www.gnu.org/licenses/
> > > > .
> > > +*/
> > > +#include <config.h>
> > > +
> > > +#include "red-channel.h"
> > > +#include "red-pipe-item.h"
> > > +
> > > +PipeItem *pipe_item_ref(gpointer object)
> > > +{
> > > +    PipeItem *item = object;
> > > +
> > > +    g_return_val_if_fail(item->refcount > 0, NULL);
> > > +
> > > +    g_atomic_int_inc(&item->refcount);
> > > +
> > > +    return item;
> > > +}
> > > +
> > > +void pipe_item_unref(gpointer object)
> > > +{
> > > +    PipeItem *item = object;
> > > +
> > > +    g_return_if_fail(item->refcount > 0);
> > > +
> > > +    if (g_atomic_int_dec_and_test(&item->refcount)) {
> > > +        item->free_func(item);
> > > +    }
> > > +}
> > > +
> > > +void pipe_item_init_full(PipeItem *item,
> > > +                         gint type,
> > > +                         GDestroyNotify free_func)
> > > +{
> > > +    ring_item_init(&item->link);
> > > +    item->type = type;
> > > +    item->refcount = 1;
> > > +    item->free_func = free_func ? free_func : (GDestroyNotify)free;
> > > +}
> > > diff --git a/server/red-pipe-item.h b/server/red-pipe-item.h
> > > new file mode 100644
> > > index 0000000..c2346f1
> > > --- /dev/null
> > > +++ b/server/red-pipe-item.h
> > > @@ -0,0 +1,57 @@
> > > +/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
> > > +/*
> > > +   Copyright (C) 2015 Red Hat, Inc.
> > > +
> > > +   This library is free software; you can redistribute it and/or
> > > +   modify it under the terms of the GNU Lesser General Public
> > > +   License as published by the Free Software Foundation; either
> > > +   version 2.1 of the License, or (at your option) any later version.
> > > +
> > > +   This library is distributed in the hope that it will be useful,
> > > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > +   Lesser General Public License for more details.
> > > +
> > > +   You should have received a copy of the GNU Lesser General Public
> > > +   License along with this library; if not, see
> > > <http://www.gnu.org/licenses/
> > > > .
> > > +*/
> > > +#ifndef _H_RED_PIPE_ITEM
> > > +#define _H_RED_PIPE_ITEM
> > > +
> > > +#include <glib.h>
> > > +
> > > +/* Refcounted pipe item */
> > > +/* Future goals are to:
> > > + * - stop having RingItem embedded at the beginning of the PipeItem base
> > > class
> > > + *   but instead have:
> > > + *   {
> > > + *       int type;
> > > + *       int refcount;
> > > + *       PipeItem link;
> > > + *   }
> > > + *   or even drop PipeItem, and use a GList in RedChannel
> > > + */
> > 
> > This comment no longer really makes sense due to the changes you made.
> > 
> 
> I'll remove. The GList part is on a commit later.
> 
> > > +typedef struct {
> > > +    RingItem link;
> > > +    int type;
> > > +
> > > +    /* private */
> > > +    int refcount;
> > > +
> > > +    GDestroyNotify free_func;
> > > +} PipeItem;
> > > +
> > > +void pipe_item_init_full(PipeItem *item, int type, GDestroyNotify
> > > free_func);
> > > +PipeItem *pipe_item_ref(gpointer item);
> > > +void pipe_item_unref(gpointer item);
> > > +
> > > +static inline int pipe_item_is_linked(PipeItem *item)
> > > +{
> > > +    return ring_item_is_linked(&item->link);
> > > +}
> > > +
> > > +static inline void pipe_item_init(PipeItem *item, int type)
> > > +{
> > > +    pipe_item_init_full(item, type, NULL);
> > > +}
> > > +#endif
> > 
> > 
> > In general, I like the changes. It's less confusing to have two different
> > pipeitem types and it doesn't look like it will cost too much. On the other
> > hand, I do prefer the RedPipeItem name, but that could be done later I
> > suppose.
> > 
> > Reviewd-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > 
> 
> For the name it's the same, I kept without as other classes don't have the
> prefix and as was unchanged.
> I prefer to do as one step instead of two.
> Probably whoever will require some more indentation.
> I'll send an update. Tomorrow. Or feel free to rename it if you prefer.
> 
> Frediano


I'll take care of it (and the comment above)

I'll do a mass rename at the end of this patch series

Jonathon

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://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]