[Spice-devel] [PATCH spice-server v2 7/8] Convert RedChannel heirarchy to GObject
Jonathon Jongsma
jjongsma at redhat.com
Tue Oct 11 20:39:44 UTC 2016
On Tue, 2016-10-11 at 16:13 +0200, Pavel Grunt wrote:
> Hi Frediano,
>
> On Tue, 2016-10-11 at 09:55 -0400, Frediano Ziglio wrote:
> >
> > >
> > >
> > > 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(
> > > > > C
> > > > > OMMON
> > > > > _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_dis
> > > > > c
> > > > > onnec
> > > > > 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_GRAP
> > > > > H
> > > > > ICS_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...
>
> Maybe we can use G_DECLARE_FINAL_TYPE
>
> https://developer.gnome.org/gobject/unstable/gobject-Type-Information
> .
> html#G-DECLARE-FINAL-TYPE:CAPS
>
> In general I prefer to expose declarations only if needed
>
> Pavel
This would still expose the same declarations, it would just hide it
behind a macro, I think. In addition, this particular macro was
introduced in glib 2.44, and we currently only require 2.22.
More information about the Spice-devel
mailing list