On Mon, Mar 21, 2016 at 02:12:49PM -0400, Frediano Ziglio wrote:
> Just for coherence I would change the case from gobject to GObject
>
> >
> > From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> >
> > ---
> > server/char-device.c | 586
> > +++++++++++++++++++++++++++++++++------------------
> > server/char-device.h | 38 +++-
> > 2 files changed, 422 insertions(+), 202 deletions(-)
> >
> > diff --git a/server/char-device.c b/server/char-device.c
> > index d2f1ea3..a74626a 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>
> > @@ -46,7 +46,7 @@ struct SpiceCharDeviceClientState {
> > uint32_t max_send_queue_size;
> > };
> >
> > -struct SpiceCharDeviceState {
> > +struct RedCharDevicePrivate {
> > int running;
> > int active; /* has read/write been performed since the device was
> > started */
> > int wait_for_migrate_data;
>
> Why this name change (Spice -> Red) ?
I don't remember. Probably because the Spice namespace is used in the
public interface, and Red internally? Or maybe it was more consistent
with the naming of the other related classes (child/parent).
> > @@ -225,14 +249,15 @@ static void
> > spice_char_device_client_free(SpiceCharDeviceState *dev,
> > RingItem *item, *next;
> >
> > if (dev_client->wait_for_tokens_timer) {
> > - reds_core_timer_remove(dev->reds,
> > dev_client->wait_for_tokens_timer);
> > + reds_core_timer_remove(dev->priv->reds,
> > dev_client->wait_for_tokens_timer);
> > + dev_client->wait_for_tokens_timer = NULL;
>
> I like this change but it's hidden by all priv changes.
> Perhaps better to put in a separate patch.
Or change reds_core_timer_remove to take a SpiceTimer **timer which it
will set to NULL by itself.
> > +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);
> > + self->priv->cur_pool_size = 0;
> > + reds_core_timer_remove(self->priv->reds,
> > self->priv->write_to_dev_timer);
> > + write_buffers_queue_free(&self->priv->write_queue);
> > + write_buffers_queue_free(&self->priv->write_bufs_pool);
> > + if (self->priv->cur_write_buf) {
> > + spice_char_device_write_buffer_free(self->priv->cur_write_buf);
> > + }
> > +
> > + 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);
> > +}
> > +
>
> This finalize is a bit different from the original destroy (order and some
> if). Could be that just this patch was based on old code and all rebases
> make the two function differs. Christophe.. do you remember any specific
> reason why this new finalize and old destroy are different?
They were initially identical, see
spice_char_device_state_destroy and red_char_device_finalize in
https://cgit.freedesktop.org/cgit/?url=~teuf/spice/commit/&h=replay-rebase&id=dff290346a6203d41b8a6fc4cdd65d0bcc344f0d
void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
{
- reds_on_char_device_state_destroy(char_dev->reds, char_dev);
- if (char_dev->write_to_dev_timer) {
- reds_core_timer_remove(char_dev->reds, char_dev->write_to_dev_timer);
- }
- write_buffers_queue_free(&char_dev->write_queue);
- write_buffers_queue_free(&char_dev->write_bufs_pool);
- if (char_dev->cur_write_buf) {
- spice_char_device_write_buffer_free(char_dev->cur_write_buf);
- }
-
- while (!ring_is_empty(&char_dev->clients)) {
- RingItem *item = ring_get_tail(&char_dev->clients);
- SpiceCharDeviceClientState *dev_client;
-
- dev_client = SPICE_CONTAINEROF(item, SpiceCharDeviceClientState, link);
- spice_char_device_client_free(char_dev, dev_client);
- }
- char_dev->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 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);
+ }
+ write_buffers_queue_free(&self->priv->write_queue);
+ write_buffers_queue_free(&self->priv->write_bufs_pool);
+ if (self->priv->cur_write_buf) {
+ spice_char_device_write_buffer_free(self->priv->cur_write_buf);
+ }
+
+ 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);
+}
+
The current difference comes from
2832fdf25 char-device: Define a memory pool limit
d7bee1bc5 char-device: fix usage of free/unref on WriteBuffer
c429574bb char-device: set to NULL freed pointers on destroy
so changing finalize() as you did in the next iteration is correct.
> > +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",
> > + "Char device
> > instance",
> > + "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",
> > + "RedsState
> > instance",
>
> In many other patches the nick as the same value as the name.
Yeah, I don't think it's meant to be that way, I usually read it as "shorter
version of the description". I don't mind if they are the same.
> > + /* FIXME: redundant with the "opaque" property in g_object_new */
> > + red_char_device_set_callbacks(char_dev, cbs, opaque);
> > +
>
> I think the solution would be add a new "cbs" properties and pass to g_object_new.
> Or I would change the FIXME to a TODO.
I this is changed later on in the series by
"Move SpiceCharDeviceCallbacks into RedCharDeviceClass"
> > +void
> > +red_char_device_set_callbacks(RedCharDevice *dev,
> > + SpiceCharDeviceCallbacks *cbs,
> > + gpointer opaque)
>
> Why not using a static function?
This is used by all children classes (before being removed).
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel