[Spice-devel] [PATCH spice-server v2 7/8] Convert RedChannel heirarchy to GObject
Jonathon Jongsma
jjongsma at redhat.com
Mon Oct 10 22:30:36 UTC 2016
On Mon, 2016-10-10 at 12:48 -0400, Frediano Ziglio wrote:
> >
> > Subject: [Spice-devel] [PATCH spice-server v2 7/8] Convert
> > RedChannel heirarchy to GObject
> >
>
> Small spell, it's "hierarchy".
>
> >
> > From: Jonathon Jongsma <jjongsma at redhat.com>
> >
> > FIXME: this commit appears to introduce a vdagent-related crash in
> > vdi_port_read_one_msg_from_device(). sin->st is NULL.
>
> If this is still true I would fix it.
I added this comment a long time ago when I was running into a crash
while testing the refactory branch. I bisected and ended up at this
commit, but didn't have time to fix the crash right away, so I added
the comment to remind myself. However, I have not seen the crash at all
recently.
My theory is that perhaps this crash was caused by a bad rebase that
introduced an error that we've subsequently fixed due to reviews and
additional rebasing.
>
> > @@ -381,7 +367,7 @@ void cursor_channel_process_cmd(CursorChannel
> > *cursor,
> > RedCursorCmd *cursor_cmd)
> >
> > void cursor_channel_reset(CursorChannel *cursor)
> > {
> > - RedChannel *channel = &cursor->common.base;
> > + RedChannel *channel = RED_CHANNEL(cursor);
> >
> > spice_return_if_fail(cursor);
> >
> > @@ -395,9 +381,9 @@ void cursor_channel_reset(CursorChannel
> > *cursor)
> > if
> > (!common_graphics_channel_get_during_target_migrate(COMMON
> > _GRAPHICS_CHANNEL(cursor)))
> > {
> > red_pipes_add_verb(channel, SPICE_MSG_CURSOR_RESET);
> > }
> > - if (!red_channel_wait_all_sent(&cursor->common.base,
> > + if (!red_channel_wait_all_sent(RED_CHANNEL(cursor),
> > COMMON_CLIENT_TIMEOUT)) {
> > - red_channel_apply_clients(channel,
> > + red_channel_apply_clients(RED_CHANNEL(cursor),
>
> Why not using channel variable ??
>
Strange. Not sure. Will change.
> >
> > red_channel_client_disconnec
> > t_if_pending_send);
> > }
> > }
> > @@ -407,7 +393,7 @@ static void
> > cursor_channel_init_client(CursorChannel
> > *cursor, CursorChannelClien
> > {
> > spice_return_if_fail(cursor);
> >
> > - if (!red_channel_is_connected(&cursor->common.base)
> > + if (!red_channel_is_connected(RED_CHANNEL(cursor))
> > ||
> > common_graphics_channel_get_during_target_migrate(COMMON_GRAPHICS_C
> > HANNEL(cursor)))
> > || {
> > spice_debug("during_target_migrate: skip init");
> > return;
> > @@ -420,7 +406,7 @@ static void
> > cursor_channel_init_client(CursorChannel
> > *cursor, CursorChannelClien
> > red_channel_pipes_add_type(RED_CHANNEL(cursor),
> > RED_PIPE_ITEM_TYPE_CURSOR_INIT);
> > }
> >
> > -void cursor_channel_init(CursorChannel *cursor)
> > +void cursor_channel_do_init(CursorChannel *cursor)
> > {
> > cursor_channel_init_client(cursor, NULL);
> > }
> > @@ -454,3 +440,25 @@ void cursor_channel_connect(CursorChannel
> > *cursor,
> > RedClient *client, RedsStream
> >
> > cursor_channel_init_client(cursor, ccc);
> > }
> > +
> > +static void
> > +cursor_channel_class_init(CursorChannelClass *klass)
> > +{
> > + RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
> > +
> > + g_type_class_add_private(klass, sizeof(CursorChannelPrivate));
> > +
> > + channel_class->parser =
> > spice_get_client_channel_parser(SPICE_CHANNEL_CURSOR, NULL);
> > + channel_class->handle_parsed =
> > red_channel_client_handle_message;
> > +
> > + channel_class->on_disconnect
> > = cursor_channel_client_on_disconnect;
> > + channel_class->send_item = cursor_channel_send_item;
> > +}
> > +
> > +static void
> > +cursor_channel_init(CursorChannel *self)
> > +{
> > + self->priv = CURSOR_CHANNEL_PRIVATE(self);
> > + self->priv->cursor_visible = TRUE;
> > + self->priv->mouse_mode = SPICE_MOUSE_MODE_SERVER;
> > +}
> > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > index a3ddaa3..8b3bc17 100644
> > --- a/server/cursor-channel.h
> > +++ b/server/cursor-channel.h
> > @@ -19,6 +19,17 @@
> > # define CURSOR_CHANNEL_H_
> >
> > #include "common-graphics-channel.h"
> > +#include "red-parse-qxl.h"
> > +
> > +G_BEGIN_DECLS
> > +
> > +#define TYPE_CURSOR_CHANNEL cursor_channel_get_type()
> > +
> > +#define CURSOR_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),
> > TYPE_CURSOR_CHANNEL, CursorChannel))
> > +#define CURSOR_CHANNEL_CLASS(klass)
> > (G_TYPE_CHECK_CLASS_CAST((klass),
> > TYPE_CURSOR_CHANNEL, CursorChannelClass))
> > +#define IS_CURSOR_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj),
> > TYPE_CURSOR_CHANNEL))
> > +#define IS_CURSOR_CHANNEL_CLASS(klass)
> > (G_TYPE_CHECK_CLASS_TYPE((klass),
> > TYPE_CURSOR_CHANNEL))
> > +#define CURSOR_CHANNEL_GET_CLASS(obj)
> > (G_TYPE_INSTANCE_GET_CLASS((obj),
> > TYPE_CURSOR_CHANNEL, CursorChannelClass))
> >
> > /**
> > * This type it's a RedChannel class which implement cursor
> > (mouse)
> > @@ -26,6 +37,22 @@
> > * A pointer to CursorChannel can be converted to a RedChannel.
> > */
> > typedef struct CursorChannel CursorChannel;
> > +typedef struct CursorChannelClass CursorChannelClass;
> > +typedef struct CursorChannelPrivate CursorChannelPrivate;
> > +
> > +struct CursorChannel
> > +{
> > + CommonGraphicsChannel parent;
> > +
> > + CursorChannelPrivate *priv;
> > +};
> > +
> > +struct CursorChannelClass
> > +{
> > + CommonGraphicsChannelClass parent_class;
> > +};
> > +
> > +GType cursor_channel_get_type(void) G_GNUC_CONST;
> >
>
> Why we need to expose this here ? On the C file is not enough ?
This is used above in all of the 'standard' GObject macros (e.g.
CURSOR_CHANNEL(), IS_CURSOR_CHANNEL(), etc). It's standard practice to
expose it in the header. In this case, it's possible that we could move
it down to the .c file, depending on whether any other files use these
macros, but I don't see much advantage to doing so. I'd prefer to leave
the standard GObject boilerplate here.
> > @@ -185,8 +185,9 @@ static void
> > red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
> > SpiceImage
> > *image,
> > SpiceImage
> > *io_image,
> > int is_lossy)
> > {
> > + DisplayChannel *display_channel =
> > + DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
> > DisplayChannelClient *dcc = DISPLAY_CHANNEL_CLIENT(rcc);
> > - DisplayChannel *display_channel = DCC_TO_DC(dcc);
> >
>
> I prefer the old version, DCC_TO_DC is still present and used.
Hmm, I guess I prefer the new version, even though it's a little more
verbose. I generally prefer using standard naming conventions instead
of excessive abbreviations that can be unclear (DCC_TO_DC). In fact I'd
prefer to eventually get rid of the use of 'dcc' altogether and switch
to DisplayChannelClient.
>
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 69edd35..19f0aca 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -20,7 +20,131 @@
> >
> > #include <common/sw_canvas.h>
> >
> > -#include "display-channel.h"
> > +#include "display-channel-private.h"
> > +
> > +G_DEFINE_TYPE(DisplayChannel, display_channel,
> > TYPE_COMMON_GRAPHICS_CHANNEL)
> > +
> > +enum {
> > + PROP0,
> > + PROP_N_SURFACES,
> > + PROP_VIDEO_CODECS
> > +};
> > +
> > +static void
> > +display_channel_get_property(GObject *object,
> > + guint property_id,
> > + GValue *value,
> > + GParamSpec *pspec)
> > +{
> > + DisplayChannel *self = DISPLAY_CHANNEL(object);
> > +
> > + switch (property_id)
> > + {
> > + case PROP_N_SURFACES:
> > + g_value_set_uint(value, self->priv->n_surfaces);
> > + break;
> > + default:
> > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > pspec);
> > + }
> > +}
> > +
> > +static void
> > +display_channel_set_property(GObject *object,
> > + guint property_id,
> > + const GValue *value,
> > + GParamSpec *pspec)
> > +{
> > + DisplayChannel *self = DISPLAY_CHANNEL(object);
> > +
> > + switch (property_id)
> > + {
> > + case PROP_N_SURFACES:
> > + self->priv->n_surfaces = g_value_get_uint(value);
> > + break;
>
> Sure we want this writable ?
> Shouldn't we reallocate the array ?
Well, if you look at where the property is defined, it's defined as a
construct-only parameter. So although it is technically writable, it's
only writable at construction. So it can't really be written after
that.
>
> > @@ -2113,6 +2204,50 @@ void
> > display_channel_reset_image_cache(DisplayChannel
> > *self)
> > image_cache_reset(&self->priv->image_cache);
> > }
> >
> > +static void
> > +display_channel_class_init(DisplayChannelClass *klass)
> > +{
> > + GObjectClass *object_class = G_OBJECT_CLASS(klass);
> > + RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
> > +
> > + g_type_class_add_private(klass,
> > sizeof(DisplayChannelPrivate));
> > +
> > + object_class->get_property = display_channel_get_property;
> > + object_class->set_property = display_channel_set_property;
> > + object_class->constructed = display_channel_constructed;
> > + object_class->finalize = display_channel_finalize;
> > +
> > + channel_class->parser =
> > spice_get_client_channel_parser(SPICE_CHANNEL_DISPLAY, NULL);
> > + channel_class->handle_parsed = dcc_handle_message;
> > +
> > + channel_class->on_disconnect = on_disconnect;
> > + channel_class->send_item = dcc_send_item;
> > + channel_class->handle_migrate_flush_mark =
> > handle_migrate_flush_mark;
> > + channel_class->handle_migrate_data = handle_migrate_data;
> > + channel_class->handle_migrate_data_get_serial =
> > handle_migrate_data_get_serial;
> > + channel_class->config_socket = dcc_config_socket;
> > +
> > + g_object_class_install_property(object_class,
> > + PROP_N_SURFACES,
> > + g_param_spec_uint("n-
> > surfaces",
> > + "number of
> > surfaces",
> > + "Number of
> > surfaces
> > for this channel",
> > + 0,
> > G_MAXUINT,
> > + 0,
> > + G_PARAM_CONS
> > TRUCT_ONLY
> > >
> > >
> > + G_PARAM_READ
> > WRITE |
> > +
>
> Imo should be write only.
>
As mentioned above, G_PARAM_CONSTRUCT_ONLY restricts it to be writable
only at construct time. But it needs to be READWRITE if we want to pass
it as a construction parameter.
> >
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 3762e54..9ac9046 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -20,32 +20,59 @@
> >
> > #include <setjmp.h>
> > #include <common/rect.h>
> > +#include <common/sw_canvas.h>
> >
> > -#include "reds-stream.h"
> > #include "cache-item.h"
> > -#include "pixmap-cache.h"
> > -#include "stat.h"
> > -#include "reds.h"
> > -#include "memslot.h"
> > -#include "red-parse-qxl.h"
> > -#include "red-record-qxl.h"
> > +#include "image-encoders.h"
> > +#include "dcc.h"
> > #include "demarshallers.h"
> > -#include "red-channel.h"
> > -#include "red-qxl.h"
> > #include "dispatcher.h"
> > #include "main-channel.h"
> > -#include "migration-protocol.h"
> > #include "main-dispatcher.h"
> > +#include "memslot.h"
> > +#include "migration-protocol.h"
> > +#include "video-encoder.h"
> > +#include "pixmap-cache.h"
> > +#include "red-channel.h"
> > +#include "red-qxl.h"
> > +#include "red-parse-qxl.h"
> > +#include "red-record-qxl.h"
> > +#include "reds-stream.h"
> > +#include "reds.h"
> > #include "spice-bitmap-utils.h"
> > -#include "image-cache.h"
> > -#include "utils.h"
> > -#include "tree.h"
> > +#include "stat.h"
> > #include "stream.h"
> > -#include "dcc.h"
> > -#include "image-encoders.h"
> > #include "common-graphics-channel.h"
> > +#include "tree.h"
> > +#include "utils.h"
> > +
>
> May headers are just included in a different order.
> Is there any reason to change the order?
>
I suspect that I may have alphabetized them here since there were
already significant changes. But we could revert that.
> >
> > diff --git a/server/dummy-channel.c b/server/dummy-channel.c
> > new file mode 100644
> > index 0000000..6ec7842
> > --- /dev/null
> > +++ b/server/dummy-channel.c
> > @@ -0,0 +1,58 @@
> > +/* dummy-channel.c */
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#include "dummy-channel.h"
> > +
> > +G_DEFINE_TYPE(DummyChannel, dummy_channel, RED_TYPE_CHANNEL)
> > +
> > +#define DUMMY_CHANNEL_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE((o),
> > TYPE_DUMMY_CHANNEL, DummyChannelPrivate))
> > +
> > +struct DummyChannelPrivate
> > +{
> > + gpointer padding;
> > +};
> > +
>
> Why just not using a private ? This padding field is not used.
> And you can just keep the priv field to retain ABI.
> This dummy stuff is moving from ugly to disgusting...
yeah, I'm not sure why I did that, to be honest.
>
> > diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> > index 83c1360..c351dad 100644
> > --- a/server/inputs-channel.c
> > +++ b/server/inputs-channel.c
> > @@ -57,6 +57,83 @@
> > #define RECEIVE_BUF_SIZE \
> > (4096 + (REDS_AGENT_WINDOW_SIZE +
> > REDS_NUM_INTERNAL_AGENT_MESSAGES) *
> > SPICE_AGENT_MAX_DATA_SIZE)
> >
> > +G_DEFINE_TYPE(InputsChannel, inputs_channel, RED_TYPE_CHANNEL)
> > +
> > +#define CHANNEL_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE((o),
> > TYPE_INPUTS_CHANNEL, InputsChannelPrivate))
> > +
> > +struct InputsChannelPrivate
> > +{
> > + uint8_t recv_buf[RECEIVE_BUF_SIZE];
> > + VDAgentMouseState mouse_state;
> > + int src_during_migrate;
> > + SpiceTimer *key_modifiers_timer;
> > +
> > + SpiceKbdInstance *keyboard;
> > + SpiceMouseInstance *mouse;
> > + SpiceTabletInstance *tablet;
> > +};
> > +
> > +
> > +static void
> > +inputs_channel_get_property(GObject *object,
> > + guint property_id,
> > + GValue *value,
> > + GParamSpec *pspec)
> > +{
> > + switch (property_id)
> > + {
> > + default:
> > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > pspec);
> > + }
> > +}
> > +
> > +static void
> > +inputs_channel_set_property(GObject *object,
> > + guint property_id,
> > + const GValue *value,
> > + GParamSpec *pspec)
> > +{
> > + switch (property_id)
> > + {
> > + default:
> > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id,
> > pspec);
> > + }
> > +}
> > +
>
> I noted there are these empty function here and in different file.
> Don't GObject provide some default functions like these?
In general, GObject is a pain and has a lot of boilerplate. So I often
use a GObject "generator" application that creates a bunch of function
skeletons. In this case I simply forgot to delete the ones I didn't
need.
>
> >
> > +static void inputs_connect(RedChannel *channel, RedClient *client,
> > + RedsStream *stream, int migration,
> > + int num_common_caps, uint32_t
> > *common_caps,
> > + int num_caps, uint32_t *caps);
> > +static void inputs_migrate(RedChannelClient *rcc);
> > +static void key_modifiers_sender(void *opaque);
> > +
> > +static void
> > +inputs_channel_constructed(GObject *object)
> > +{
> > + ClientCbs client_cbs = { NULL, };
> > + InputsChannel *self = INPUTS_CHANNEL(object);
> > + RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> > +
> > + G_OBJECT_CLASS(inputs_channel_parent_class)-
> > >constructed(object);
> > +
> > + client_cbs.connect = inputs_connect;
> > + client_cbs.migrate = inputs_migrate;
> > + red_channel_register_client_cbs(RED_CHANNEL(self),
> > &client_cbs, NULL);
> > +
> > + red_channel_set_cap(RED_CHANNEL(self),
> > SPICE_INPUTS_CAP_KEY_SCANCODE);
> > + reds_register_channel(reds, RED_CHANNEL(self));
> > +
> > + if (!(self->priv->key_modifiers_timer =
> > reds_core_timer_add(reds,
> > key_modifiers_sender, self))) {
> > + spice_error("key modifiers timer create failed");
> > + }
> > +}
> > +
> > +static void
> > +inputs_channel_init(InputsChannel *self)
> > +{
> > + self->priv = CHANNEL_PRIVATE(self);
> > +}
> > +
> > struct SpiceKbdState {
> > uint8_t push_ext_type;
> >
> > @@ -105,18 +182,6 @@ RedsState*
> > spice_tablet_state_get_server(SpiceTabletState *st)
> > return st->reds;
> > }
> >
> > -struct InputsChannel {
> > - RedChannel base;
> > - uint8_t recv_buf[RECEIVE_BUF_SIZE];
> > - VDAgentMouseState mouse_state;
> > - int src_during_migrate;
> > - SpiceTimer *key_modifiers_timer;
> > -
> > - SpiceKbdInstance *keyboard;
> > - SpiceMouseInstance *mouse;
> > - SpiceTabletInstance *tablet;
> > -};
> > -
>
> Couldn't we move new function after this declaration to reduce patch
> size?
Yeah, I'll try to reduce the diff a bit more.
>
> >
> > typedef struct RedKeyModifiersPipeItem {
> > RedPipeItem base;
> > uint8_t modifiers;
>
> ... omissis ...
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list