[Spice-devel,v3] Add features to PipeItem class

Submitted by Frediano Ziglio on April 14, 2016, 9:54 a.m.

Details

Message ID 1460627645-22651-1-git-send-email-fziglio@redhat.com
State New
Headers show
Series "Backported patches from refactory branch (April 13)" ( rev: 4 ) in Spice

Browsing this patch as part of:
"Backported patches from refactory branch (April 13)" rev 4 in Spice
<< prev patch [13/13] next patch >>

Commit Message

Frediano Ziglio April 14, 2016, 9:54 a.m.
From: Christophe Fergeau <cfergeau@redhat.com>

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@redhat.com>
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 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).

Patch hide | download patch | download mbox

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
+ */
+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

Comments

On Thu, 2016-04-14 at 10:54 +0100, Frediano Ziglio wrote:
> From: Christophe Fergeau <cfergeau@redhat.com>
> 
> 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@redhat.com>
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>  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.

> +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@redhat.com>
> 
> On Thu, 2016-04-14 at 10:54 +0100, Frediano Ziglio wrote:
> > From: Christophe Fergeau <cfergeau@redhat.com>
> > 
> > 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@redhat.com>
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
> > ---
> >  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@redhat.com>
> 

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
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@redhat.com>
> > > 
> > > 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@redhat.com>
> > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
> > > ---
> > >  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@redhat.com>
> > 
> 
> 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