[Spice-devel] [PATCH spice-server v2 7/8] Convert RedChannel heirarchy to GObject
Frediano Ziglio
fziglio at redhat.com
Tue Oct 11 13:55:22 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.
>
Some months ago my sister decided it was time to change the car.
As she has some child she decided for a bigger car. She was also thinking
about having additional space to allow to go to holidays. After viewing
different car we discussed if it was worth buying a bigger car considering
it was more expensive to buy and to maintain (insurance and fuel).
At the end she decided to buy a car enough for the child and family
but that was not worth buying some more big one just for the holiday
opting for a car roof bag.
The 'standard' solution for the car is buying a bigger car but not
all standard solutions are the best.
The main reason why the standard is that way is that it supports more
cases and so it's easier to put in a guide or tutorial or a book...
just that we are not writing a book or following a tutorial or guide.
Exposing CursorChannel and CursorChannelClass in the header is a way
to tell that this is the object structure and can be used for instance
for inheritance. Some languages introduced the "final" keyword to avoid
inheritance... C just do not support objects so not exposing the
structure it's a way to use.
Also is there is no need to expose the structure why to do it?
Another reason in favor of not exposing it is that you could avoid to
pay the penalty (memory, code and cpu) of the "priv" field.
About all that macros boilerplate I don't understand why they
don't came to a better solution, something like
G_TYPE_CAST(pointer, RedChannel)
instead of having to define all these macro for every object...
Require that if there is a GObject typename "RedChannel" you
have a RedChannel_get_type() function does not seem so hard
or cause so many problems (but probably I miss something).
Also instead of
if (IS_CURSOR_CHANNEL(obj))
why not using
if (CURSOR_CHANNEL(obj))
and avoid to define 2 macros?
As macro only users of these headers can use them and we
are the only users.
So can't we define our macros so we don't have all that
boilerplate?
Are we expecting to export these GObject outside our code or
use then in an higher level language such as Java or a visual
tool?
>
> > > @@ -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.
>
Yes, you are right.
> > >
> > > 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.
>
I personally don't like that much generators if they generate too
much code as you'll have to maintain lot of code.
In this case for instance if the next version of GObject decide to
change the way to implement properties you'll have to update all
that code.
Just to make clear I don't consider programs like bison/flex/gperf
as generators as you don't expect to maintain their output but
you just use them in the chain.
> >
> > >
> > > +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
More information about the Spice-devel
mailing list