On Wed, 2016-03-30 at 14:25 -0500, Jonathon Jongsma wrote:
> On Wed, 2016-03-30 at 18:20 +0100, Frediano Ziglio wrote:
> > From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> >
> > ---
> > 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@xxxxxxxxxx>
> > @@ -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 */
...and now I notice this comment. I still think that we should convert all
internal usage to RedCharDevice, however.
> > +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@xxxxxxxxxx>
>
> [1] https://lists.freedesktop.org/archives/spice-devel/2016-March/027850.html
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel