[Spice-devel] [PATCH v3] Add features to PipeItem class
Jonathon Jongsma
jjongsma at redhat.com
Thu Apr 14 16:37:07 UTC 2016
On Thu, 2016-04-14 at 10:54 +0100, Frediano Ziglio wrote:
> From: Christophe Fergeau <cfergeau at 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 at redhat.com>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> Reviewed-by: Jonathon Jongsma <jjongsma at 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 at redhat.com>
More information about the Spice-devel
mailing list