Re: [PATCH 03/14] Introduce simple RedPipeItem class

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

 



On Thu, 2016-04-07 at 17:11 -0500, Jonathon Jongsma wrote:
> From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> 
> This is similar to PipeItem (which is a type id + a RingItem linked list
> member), except that it's refcounted. A user-defined callback is called
> when the refcount drops to 0.

It's not so much "like" a PipeItem -- it actually "contains" a PipeItem or
"inherits" from PipeItem.

> Refcounted is open coded for several classes deriving from PipeItem, so

I'm unfamiliar with the term "open coded". Is that a reference to the fact that
the ref/unref functions take a void* argument? If so, I'm not sure that I like
that idea very much, though I do admit that it probably makes usage a bit more
convenient...

> this base class 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.

A bit more information about why this class is useful and how it will be used in
the future would be nice here (I'm sure it will become obvious as I look at
future patches...)

> ---
>  server/Makefile.am     |  2 ++
>  server/red-channel.c   |  6 -----
>  server/red-channel.h   | 13 +---------
>  server/red-pipe-item.c | 65
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  server/red-pipe-item.h | 60 ++++++++++++++++++++++++++++++++++++++++++++++
>  server/smartcard.c     |  1 +
>  6 files changed, 129 insertions(+), 18 deletions(-)
>  create mode 100644 server/red-pipe-item.c
>  create mode 100644 server/red-pipe-item.h
> 
> 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 d8f1d27..dcec39e 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..a9ebfda
> --- /dev/null
> +++ b/server/red-pipe-item.c
> @@ -0,0 +1,65 @@
> +/* -*- 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"
> +
> +void pipe_item_init(PipeItem *item, int type)
> +{
> +    ring_item_init(&item->link);
> +    item->type = type;
> +}
> +
> +RedPipeItem *red_pipe_item_ref(gpointer object)
> +{
> +    RedPipeItem *item = object;
> +
> +    g_return_val_if_fail(item->refcount > 0, NULL);
> +
> +    item->refcount++;

Should we make these refcounts thread-safe?

> +
> +    return item;
> +}
> +
> +void red_pipe_item_unref(gpointer object)
> +{
> +    RedPipeItem *item = object;
> +
> +    g_return_if_fail(item->refcount > 0);
> +
> +    item->refcount--;
> +    if (item->refcount == 0) {
> +        if (item->free_func) {
> +            item->free_func(item);
> +        } else {
> +            free(item);
> +        }
> +    }
> +}
> +
> +void red_pipe_item_init(RedPipeItem *item,
> +                        gint type,
> +                        GDestroyNotify free_func)
> +{
> +    g_return_if_fail(item->refcount == 0);
> +
> +    pipe_item_init(&item->parent, type);
> +    item->refcount = 1;
> +    item->free_func = free_func;
> +}
> diff --git a/server/red-pipe-item.h b/server/red-pipe-item.h
> new file mode 100644
> index 0000000..7bc5abc
> --- /dev/null
> +++ b/server/red-pipe-item.h
> @@ -0,0 +1,60 @@
> +/* -*- 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 __RED_MINI_OBJECT_H__
> +#define __RED_MINI_OBJECT_H__

This symbol is probably left over from a previous iteration? Not a big deal, but
probably should match the file name.

> +
> +#include <glib.h>
> +
> +typedef struct PipeItem {
> +    RingItem link;
> +    int type;
> +} PipeItem;
> +
> +static inline int pipe_item_is_linked(PipeItem *item)
> +{
> +    return ring_item_is_linked(&item->link);
> +}
> +
> +void pipe_item_init(PipeItem *item, int type);
> +
> +/* Refcounted pipe item */
> +/* Future goals are to:
> + * - merge it with PipeItem
> + * - 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 {
> +    PipeItem parent;
> +
> +    /* private */
> +    int refcount;
> +
> +    GDestroyNotify free_func;
> +} RedPipeItem;
> +
> +RedPipeItem *red_pipe_item_ref(gpointer item);
> +void red_pipe_item_unref(gpointer item);
> +void red_pipe_item_init(RedPipeItem *item, int type, GDestroyNotify
> free_func);
> +
> +#endif
> diff --git a/server/smartcard.c b/server/smartcard.c
> index eb68a8b..557a622 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -88,6 +88,7 @@ typedef struct ErrorItem {
>      VSCMsgError  error;
>  } ErrorItem;
>  
> +

unrelated whitespace 

>  typedef struct MsgItem {
>      PipeItem base;
>      uint32_t refs;



Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
_______________________________________________
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]