[Spice-devel] [PATCH 02/10] char-device: Make SpiceCharDeviceState a GObject
Jonathon Jongsma
jjongsma at redhat.com
Wed Mar 30 19:25:07 UTC 2016
On Wed, 2016-03-30 at 18:20 +0100, Frediano Ziglio wrote:
> From: Christophe Fergeau <cfergeau at redhat.com>
>
> ---
> server/char-device.c | 352 ++++++++++++++++++++++++++++++++++++++------------
> -
> server/char-device.h | 43 ++++++-
> 2 files changed, 304 insertions(+), 91 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index 53bfe82..7fd2a0b 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -1,6 +1,6 @@
> /* spice-server char device flow control code
>
> - Copyright (C) 2012 Red Hat, Inc.
> + Copyright (C) 2012-2015 Red Hat, Inc.
>
> Red Hat Authors:
> Yonit Halperin <yhalperi at redhat.com>
> @@ -74,10 +74,6 @@ struct RedCharDevicePrivate {
> SpiceServer *reds;
> };
>
> -struct SpiceCharDeviceState {
> - struct RedCharDevicePrivate priv[1];
> -};
> -
> enum {
> WRITE_BUFFER_ORIGIN_NONE,
> WRITE_BUFFER_ORIGIN_CLIENT,
> @@ -85,12 +81,21 @@ enum {
> WRITE_BUFFER_ORIGIN_SERVER_NO_TOKEN,
> };
>
> -/* Holding references for avoiding access violation if the char device was
> - * destroyed during a callback */
> -static void spice_char_device_state_ref(SpiceCharDeviceState *char_dev);
> -static void spice_char_device_state_unref(SpiceCharDeviceState *char_dev);
> -static void spice_char_device_write_buffer_unref(SpiceCharDeviceWriteBuffer
> *write_buf);
>
> +G_DEFINE_TYPE(RedCharDevice, red_char_device, G_TYPE_OBJECT)
> +
> +#define RED_CHAR_DEVICE_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o),
> RED_TYPE_CHAR_DEVICE, RedCharDevicePrivate))
> +
> +enum {
> + PROP_0,
> + PROP_CHAR_DEV_INSTANCE,
> + PROP_SPICE_SERVER,
> + PROP_CLIENT_TOKENS_INTERVAL,
> + PROP_SELF_TOKENS,
> + PROP_OPAQUE
> +};
> +
> +static void spice_char_device_write_buffer_unref(SpiceCharDeviceWriteBuffer
> *write_buf);
> static void spice_char_dev_write_retry(void *opaque);
>
> typedef struct SpiceCharDeviceMsgToClientItem {
> @@ -98,6 +103,13 @@ typedef struct SpiceCharDeviceMsgToClientItem {
> SpiceCharDeviceMsgToClient *msg;
> } SpiceCharDeviceMsgToClientItem;
>
> +static RedCharDevice *red_char_device_new(SpiceCharDeviceInstance *sin,
> + RedsState *reds,
> + uint32_t client_tokens_interval,
> + uint32_t self_tokens,
> + SpiceCharDeviceCallbacks *cbs,
> + void *opaque);
> +
> static SpiceCharDeviceMsgToClient *
> spice_char_device_read_one_msg_from_device(SpiceCharDeviceState *dev)
> {
> @@ -364,7 +376,7 @@ static int
> spice_char_device_read_from_device(SpiceCharDeviceState *dev)
> }
>
> max_send_tokens = spice_char_device_max_send_tokens(dev);
> - spice_char_device_state_ref(dev);
> + g_object_ref(dev);
> /*
> * Reading from the device only in case at least one of the clients have
> a free token.
> * All messages will be discarded if no client is attached to the device
> @@ -390,7 +402,7 @@ static int
> spice_char_device_read_from_device(SpiceCharDeviceState *dev)
> if (dev->priv->running) {
> dev->priv->active = dev->priv->active || did_read;
> }
> - spice_char_device_state_unref(dev);
> + g_object_unref(dev);
> return did_read;
> }
>
> @@ -507,7 +519,7 @@ static int
> spice_char_device_write_to_device(SpiceCharDeviceState *dev)
> return 0;
> }
>
> - spice_char_device_state_ref(dev);
> + g_object_ref(dev);
>
> if (dev->priv->write_to_dev_timer) {
> reds_core_timer_cancel(dev->priv->reds, dev->priv
> ->write_to_dev_timer);
> @@ -561,7 +573,7 @@ static int
> spice_char_device_write_to_device(SpiceCharDeviceState *dev)
> dev->priv->active = dev->priv->active || total;
> }
> dev->priv->during_write_to_device = 0;
> - spice_char_device_state_unref(dev);
> + g_object_unref(dev);
> return total;
> }
>
> @@ -726,39 +738,8 @@ SpiceCharDeviceState
> *spice_char_device_state_create(SpiceCharDeviceInstance *si
> SpiceCharDeviceCallbacks
> *cbs,
> void *opaque)
> {
> - SpiceCharDeviceState *char_dev;
> - SpiceCharDeviceInterface *sif;
> -
> - spice_assert(sin);
> - spice_assert(cbs->read_one_msg_from_device && cbs->ref_msg_to_client &&
> - cbs->unref_msg_to_client && cbs->send_msg_to_client &&
> - cbs->send_tokens_to_client && cbs->remove_client);
> -
> - char_dev = spice_new0(SpiceCharDeviceState, 1);
> - char_dev->priv->sin = sin;
> - char_dev->priv->reds = reds;
> - char_dev->priv->cbs = *cbs;
> - char_dev->priv->opaque = opaque;
> - char_dev->priv->client_tokens_interval = client_tokens_interval;
> - char_dev->priv->num_self_tokens = self_tokens;
> -
> - ring_init(&char_dev->priv->write_queue);
> - ring_init(&char_dev->priv->write_bufs_pool);
> - ring_init(&char_dev->priv->clients);
> -
> - sif = spice_char_device_get_interface(char_dev->priv->sin);
> - if (sif->base.minor_version <= 2 ||
> - !(sif->flags & SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
> - char_dev->priv->write_to_dev_timer = reds_core_timer_add(reds,
> spice_char_dev_write_retry, char_dev);
> - if (!char_dev->priv->write_to_dev_timer) {
> - spice_error("failed creating char dev write timer");
> - }
> - }
> -
> - char_dev->priv->refs = 1;
> - sin->st = char_dev;
> - spice_debug("sin %p dev_state %p", sin, char_dev);
> - return char_dev;
> + return red_char_device_new(sin, reds, client_tokens_interval,
> + self_tokens, cbs, opaque);
> }
>
> void spice_char_device_state_reset_dev_instance(SpiceCharDeviceState *state,
> @@ -774,48 +755,10 @@ void
> *spice_char_device_state_opaque_get(SpiceCharDeviceState *dev)
> return dev->priv->opaque;
> }
>
> -static void spice_char_device_state_ref(SpiceCharDeviceState *char_dev)
> -{
> - char_dev->priv->refs++;
> -}
> -
> -static void spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
> -{
> - /* The refs field protects the char_dev from being deallocated in
> - * case spice_char_device_state_destroy has been called
> - * during a callabck, and we might still access the char_dev afterwards.
> - * spice_char_device_state_unref is always coupled with a preceding
> - * spice_char_device_state_ref. Here, refs can turn 0
> - * only when spice_char_device_state_destroy is called in between
> - * the calls to spice_char_device_state_ref and
> spice_char_device_state_unref.*/
> - if (!--char_dev->priv->refs) {
> - free(char_dev);
> - }
> -}
> -
> void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
> {
> - reds_on_char_device_state_destroy(char_dev->priv->reds, char_dev);
> - if (char_dev->priv->write_to_dev_timer) {
> - reds_core_timer_remove(char_dev->priv->reds, char_dev->priv
> ->write_to_dev_timer);
> - char_dev->priv->write_to_dev_timer = NULL;
> - }
> - write_buffers_queue_free(&char_dev->priv->write_queue);
> - write_buffers_queue_free(&char_dev->priv->write_bufs_pool);
> - char_dev->priv->cur_pool_size = 0;
> - spice_char_device_write_buffer_free(char_dev->priv->cur_write_buf);
> - char_dev->priv->cur_write_buf = NULL;
> -
> - while (!ring_is_empty(&char_dev->priv->clients)) {
> - RingItem *item = ring_get_tail(&char_dev->priv->clients);
> - SpiceCharDeviceClientState *dev_client;
> -
> - dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState,
> link);
> - spice_char_device_client_free(char_dev, dev_client);
> - }
> - char_dev->priv->running = FALSE;
> -
> - spice_char_device_state_unref(char_dev);
> + g_return_if_fail(RED_IS_CHAR_DEVICE(char_dev));
> + g_object_unref(char_dev);
> }
>
> static SpiceCharDeviceClientState *red_char_device_client_new(RedClient
> *client,
> @@ -919,10 +862,10 @@ void spice_char_device_start(SpiceCharDeviceState *dev)
> {
> spice_debug("dev_state %p", dev);
> dev->priv->running = TRUE;
> - spice_char_device_state_ref(dev);
> + g_object_ref(dev);
> while (spice_char_device_write_to_device(dev) ||
> spice_char_device_read_from_device(dev));
> - spice_char_device_state_unref(dev);
> + g_object_unref(dev);
> }
>
> void spice_char_device_stop(SpiceCharDeviceState *dev)
> @@ -1116,3 +1059,234 @@ SpiceCharDeviceInterface
> *spice_char_device_get_interface(SpiceCharDeviceInstanc
> {
> return SPICE_CONTAINEROF(instance->base.sif, SpiceCharDeviceInterface,
> base);
> }
> +
> +
> +static void red_char_device_init_device_instance(RedCharDevice *self)
> +{
> + SpiceCharDeviceInterface *sif;
> +
> + g_return_if_fail(self->priv->reds);
> +
> + if (self->priv->write_to_dev_timer) {
> + reds_core_timer_remove(self->priv->reds, self->priv
> ->write_to_dev_timer);
> + self->priv->write_to_dev_timer = NULL;
> + }
> + if (self->priv->sin == NULL) {
> + return;
> + }
> +
> + sif = spice_char_device_get_interface(self->priv->sin);
> + if (sif->base.minor_version <= 2 ||
> + !(sif->flags & SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
> + self->priv->write_to_dev_timer = reds_core_timer_add(self->priv
> ->reds,
> +
> spice_char_dev_write_retry,
> + self);
> + if (!self->priv->write_to_dev_timer) {
> + spice_error("failed creating char dev write timer");
> + }
> + }
> +
> + self->priv->sin->st = self;
> +}
> +
> +static void
> +red_char_device_get_property(GObject *object,
> + guint property_id,
> + GValue *value,
> + GParamSpec *pspec)
> +{
> + RedCharDevice *self = RED_CHAR_DEVICE(object);
> +
> + switch (property_id)
> + {
> + case PROP_CHAR_DEV_INSTANCE:
> + g_value_set_pointer(value, self->priv->sin);
> + break;
> + case PROP_SPICE_SERVER:
> + g_value_set_pointer(value, self->priv->reds);
> + break;
> + case PROP_CLIENT_TOKENS_INTERVAL:
> + g_value_set_uint64(value, self->priv->client_tokens_interval);
> + break;
> + case PROP_SELF_TOKENS:
> + g_value_set_uint64(value, self->priv->num_self_tokens);
> + break;
> + case PROP_OPAQUE:
> + g_value_set_pointer(value, self->priv->opaque);
> + break;
> + default:
> + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> + }
> +}
> +
> +static void
> +red_char_device_set_property(GObject *object,
> + guint property_id,
> + const GValue *value,
> + GParamSpec *pspec)
> +{
> + RedCharDevice *self = RED_CHAR_DEVICE(object);
> +
> + switch (property_id)
> + {
> + case PROP_CHAR_DEV_INSTANCE:
> + self->priv->sin = g_value_get_pointer(value);
> + break;
> + case PROP_SPICE_SERVER:
> + self->priv->reds = g_value_get_pointer(value);
> + break;
> + case PROP_CLIENT_TOKENS_INTERVAL:
> + self->priv->client_tokens_interval = g_value_get_uint64(value);
> + break;
> + case PROP_SELF_TOKENS:
> + self->priv->num_self_tokens = g_value_get_uint64(value);
> + break;
> + case PROP_OPAQUE:
> + self->priv->opaque = g_value_get_pointer(value);
> + break;
> + default:
> + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> + }
> +}
> +
> +static void
> +red_char_device_on_sin_changed(GObject *object,
> + GParamSpec *pspec G_GNUC_UNUSED,
> + gpointer user_data G_GNUC_UNUSED)
> +{
> + RedCharDevice *self = RED_CHAR_DEVICE(object);
> +
> + red_char_device_init_device_instance(self);
> +}
> +
> +static void
> +red_char_device_finalize(GObject *object)
> +{
> + RedCharDevice *self = RED_CHAR_DEVICE(object);
> +
> + /* FIXME: replace with g_object_weak_ref () */
> + reds_on_char_device_state_destroy(self->priv->reds, self);
> + if (self->priv->write_to_dev_timer) {
> + reds_core_timer_remove(self->priv->reds, self->priv
> ->write_to_dev_timer);
> + self->priv->write_to_dev_timer = NULL;
> + }
> + write_buffers_queue_free(&self->priv->write_queue);
> + write_buffers_queue_free(&self->priv->write_bufs_pool);
> + self->priv->cur_pool_size = 0;
> + spice_char_device_write_buffer_free(self->priv->cur_write_buf);
> + self->priv->cur_write_buf = NULL;
> +
> + while (!ring_is_empty(&self->priv->clients)) {
> + RingItem *item = ring_get_tail(&self->priv->clients);
> + SpiceCharDeviceClientState *dev_client;
> +
> + dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState,
> link);
> + spice_char_device_client_free(self, dev_client);
> + }
> + self->priv->running = FALSE;
> +
> + G_OBJECT_CLASS(red_char_device_parent_class)->finalize(object);
> +}
> +
> +static void
> +red_char_device_class_init(RedCharDeviceClass *klass)
> +{
> + GObjectClass *object_class = G_OBJECT_CLASS(klass);
> +
> + g_type_class_add_private(klass, sizeof (RedCharDevicePrivate));
> +
> + object_class->get_property = red_char_device_get_property;
> + object_class->set_property = red_char_device_set_property;
> + object_class->finalize = red_char_device_finalize;
> +
> + g_object_class_install_property(object_class,
> + PROP_CHAR_DEV_INSTANCE,
> + g_param_spec_pointer("sin",
> + "sin",
> + "Char device
> instance",
> +
> G_PARAM_STATIC_STRINGS |
> + G_PARAM_READWRITE |
> + G_PARAM_CONSTRUCT));
> + g_object_class_install_property(object_class,
> + PROP_SPICE_SERVER,
> + g_param_spec_pointer("spice-server",
> + "spice-server",
> + "RedsState
> instance",
> +
> G_PARAM_STATIC_STRINGS |
> + G_PARAM_READWRITE |
> + G_PARAM_CONSTRUCT));
> + g_object_class_install_property(object_class,
> + PROP_CLIENT_TOKENS_INTERVAL,
> + g_param_spec_uint64("client-tokens
> -interval",
> + "client-tokens
> -interval",
> + "Client token
> interval",
> + 0, G_MAXUINT64, 0,
> +
> G_PARAM_STATIC_STRINGS |
> + G_PARAM_READWRITE));
> + g_object_class_install_property(object_class,
> + PROP_SELF_TOKENS,
> + g_param_spec_uint64("self-tokens",
> + "self-tokens",
> + "Self tokens",
> + 0, G_MAXUINT64, 0,
> +
> G_PARAM_STATIC_STRINGS |
> + G_PARAM_READWRITE));
> + g_object_class_install_property(object_class,
> + PROP_OPAQUE,
> + g_param_spec_pointer("opaque",
> + "opaque",
> + "User data to pass
> to callbacks",
> + G_PARAM_STATIC_STRINGS
> |
> + G_PARAM_READWRITE));
> +
> +}
> +
> +static void
> +red_char_device_init(RedCharDevice *self)
> +{
> + self->priv = RED_CHAR_DEVICE_PRIVATE(self);
> +
> + ring_init(&self->priv->write_queue);
> + ring_init(&self->priv->write_bufs_pool);
> + ring_init(&self->priv->clients);
> +
> + g_signal_connect(self, "notify::sin",
> G_CALLBACK(red_char_device_on_sin_changed), NULL);
> +}
> +
> +static RedCharDevice *
> +red_char_device_new(SpiceCharDeviceInstance *sin,
> + RedsState *reds,
> + uint32_t client_tokens_interval,
> + uint32_t self_tokens,
> + SpiceCharDeviceCallbacks *cbs,
> + void *opaque)
> +{
> + RedCharDevice *char_dev;
> +
> + char_dev = g_object_new(RED_TYPE_CHAR_DEVICE,
> + "sin", sin,
> + "reds", reds,
> + "client-tokens-interval", (guint64)
> client_tokens_interval,
> + "self-tokens", (guint64) self_tokens,
> + "opaque", opaque,
> + NULL);
> +
> + /* TODO: redundant with the "opaque" property in g_object_new */
> + red_char_device_set_callbacks(char_dev, cbs, opaque);
> +
> + return char_dev;
> +}
> +
> +/* TODO: needs to be moved to class vfuncs once all child classes are
> gobjects */
> +void
> +red_char_device_set_callbacks(RedCharDevice *dev,
> + SpiceCharDeviceCallbacks *cbs,
> + gpointer opaque)
> +{
> + g_assert(cbs->read_one_msg_from_device && cbs->ref_msg_to_client &&
> + cbs->unref_msg_to_client && cbs->send_msg_to_client &&
> + cbs->send_tokens_to_client && cbs->remove_client);
> +
> + dev->priv->cbs = *cbs;
> + dev->priv->opaque = opaque;
> +}
> diff --git a/server/char-device.h b/server/char-device.h
> index 7c78524..b1b5b18 100644
> --- a/server/char-device.h
> +++ b/server/char-device.h
> @@ -18,10 +18,44 @@
> #ifndef CHAR_DEVICE_H_
> #define CHAR_DEVICE_H_
>
> +#include <glib-object.h>
> +
> #include "spice.h"
> #include "red-channel.h"
> #include "migration-protocol.h"
>
> +#define RED_TYPE_CHAR_DEVICE red_char_device_get_type()
> +
> +#define RED_CHAR_DEVICE(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),
> RED_TYPE_CHAR_DEVICE, RedCharDevice))
> +#define RED_CHAR_DEVICE_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST((klass),
> RED_TYPE_CHAR_DEVICE, RedCharDeviceClass))
> +#define RED_IS_CHAR_DEVICE(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj),
> RED_TYPE_CHAR_DEVICE))
> +#define RED_IS_CHAR_DEVICE_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((klass),
> RED_TYPE_CHAR_DEVICE))
> +#define RED_CHAR_DEVICE_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS((obj),
> RED_TYPE_CHAR_DEVICE, RedCharDeviceClass))
> +
> +typedef struct SpiceCharDeviceState RedCharDevice;
As I mentioned in a review of a later patch in this series [1], I don't like
this typedef. I understand it was done so that the refactoring could be done
piecemeal, without having to change too much surrounding code. But I think it
has a significant negative impact on code readability to have several names for
different types, especially when there are already so many types with similar
names to keep track of. It doesn't have to be done in this commit, it could be
done in a preparatory or follow-up commit.
> +typedef struct RedCharDeviceClass RedCharDeviceClass;
> +typedef struct RedCharDevicePrivate RedCharDevicePrivate;
> +
> +/* 'SpiceCharDeviceState' name is used for consistency with what spice-char.h
> exports */
> +struct SpiceCharDeviceState
> +{
> + GObject parent;
> +
> + RedCharDevicePrivate *priv;
> +};
> +
> +struct RedCharDeviceClass
> +{
> + GObjectClass parent_class;
> +};
> +
> +GType red_char_device_get_type(void) G_GNUC_CONST;
> +
> +typedef struct SpiceCharDeviceCallbacks SpiceCharDeviceCallbacks;
> +void red_char_device_set_callbacks(RedCharDevice *dev,
> + SpiceCharDeviceCallbacks *cbs,
> + gpointer opaque);
> +
> /*
> * Shared code for char devices, mainly for flow control.
> *
> @@ -55,6 +89,11 @@
> * spice_char_device_stop
> * spice_char_device_wakeup (for reading from the device)
> */
> +/* refcounting is used to protect the char_dev from being deallocated in
> + * case spice_char_device_state_destroy has been called
> + * during a callback, and we might still access the char_dev afterwards.
> + */
> +
>
> /*
> * Note about multiple-clients:
> @@ -97,7 +136,7 @@ typedef struct SpiceCharDeviceWriteBuffer {
>
> typedef void SpiceCharDeviceMsgToClient;
>
> -typedef struct SpiceCharDeviceCallbacks {
> +struct SpiceCharDeviceCallbacks {
> /*
> * Messages that are addressed to the client can be queued in case we
> have
> * multiple clients and some of them don't have enough tokens.
> @@ -127,7 +166,7 @@ typedef struct SpiceCharDeviceCallbacks {
> * due to slow flow or due to some other error.
> * The called instance should disconnect the client, or at least the
> corresponding channel */
> void (*remove_client)(RedClient *client, void *opaque);
> -} SpiceCharDeviceCallbacks;
> +};
>
> SpiceCharDeviceState *spice_char_device_state_create(SpiceCharDeviceInstance
> *sin,
> struct RedsState *reds,
Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
[1] https://lists.freedesktop.org/archives/spice-devel/2016-March/027850.html
More information about the Spice-devel
mailing list