[Spice-devel] [PATCH spice-server v2 7/8] Convert RedChannel heirarchy to GObject
Frediano Ziglio
fziglio at redhat.com
Wed Oct 12 10:32:42 UTC 2016
> On Tue, 2016-10-11 at 09:55 -0400, Frediano Ziglio wrote:
> > >
> > > > > 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...
>
> These are for convenience. You could use G_TYPE_CHECK_INSTANCE_CAST()
> directly if you want, but it's nicer to use a convenience macro like
> CURSOR_CHANNEL().
>
Yes, this is mainly the only used beside some exceptions.
> > 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?
>
> It's mostly a matter of convention. Most people simply expect these
> macros to exist with a standard naming convention for any GObject type.
> It's true that not all of these macros are always used for every type.
> For instance, In this particular case we don't use them except within
> the cursor-channel.c implementation. So we could certainly move them
> down to the .c file (or remove them entirely) as you suggested. But if
> I came across a GObject implementation that didn't define the standard
> cast macros in the header, it would strike me as odd.
>
>
> > 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?
>
> We could define our own macros, I suppose. But why? Hundreds of GObject
> applications and libraries do it this way. I don't think there's much
> to be gained by bucking tradition here and implementing our own custom
> cast macros just to avoid a symbol in the header.
>
I'm more concerned about copy&paste and edit every time.
I tried a different approach and I came with this patch
diff --git a/server/red-channel-client.h b/server/red-channel-client.h
index 3a09510..9d65595 100644
--- a/server/red-channel-client.h
+++ b/server/red-channel-client.h
@@ -38,27 +38,28 @@ G_BEGIN_DECLS
#define RED_TYPE_CHANNEL_CLIENT red_channel_client_get_type()
-#define RED_CHANNEL_CLIENT(obj) \
- (G_TYPE_CHECK_INSTANCE_CAST((obj), RED_TYPE_CHANNEL_CLIENT, RedChannelClient))
-#define RED_CHANNEL_CLIENT_CLASS(klass) \
- (G_TYPE_CHECK_CLASS_CAST((klass), RED_TYPE_CHANNEL_CLIENT, RedChannelClientClass))
-#define RED_IS_CHANNEL_CLIENT(obj) \
- (G_TYPE_CHECK_INSTANCE_TYPE((obj), RED_TYPE_CHANNEL_CLIENT))
-#define RED_IS_CHANNEL_CLIENT_CLASS(klass) \
- (G_TYPE_CHECK_CLASS_TYPE((klass), RED_TYPE_CHANNEL_CLIENT))
-#define RED_CHANNEL_CLIENT_GET_CLASS(obj) \
- (G_TYPE_INSTANCE_GET_CLASS((obj), RED_TYPE_CHANNEL_CLIENT, RedChannelClientClass))
+#define GOBJECT_DECLARE_TYPE(type, prefix, macro) \
+ typedef struct type type; \
+ typedef struct type ## Class type ## Class; \
+ typedef struct type ## Private type ## Private; \
+ GType prefix ## _get_type(void) G_GNUC_CONST; \
+ static inline type *macro(void *obj) \
+ { return G_TYPE_CHECK_INSTANCE_CAST(obj, prefix ## _get_type(), type); } \
+ static inline type ## Class *macro ## _CLASS(void *klass) \
+ { return G_TYPE_CHECK_CLASS_CAST(klass, prefix ## _get_type(), type ## Class); } \
+ static inline gboolean IS_ ## macro(void *obj) \
+ { return G_TYPE_CHECK_INSTANCE_TYPE(obj, prefix ## _get_type()); } \
+ static inline gboolean IS_ ## macro ## _CLASS(void *klass) \
+ { return G_TYPE_CHECK_CLASS_TYPE((klass), prefix ## _get_type()); } \
+ static inline type ## Class *macro ## _GET_CLASS(void *obj) \
+ { return G_TYPE_INSTANCE_GET_CLASS(obj, prefix ## _get_type(), type ## Class); }
+
+GOBJECT_DECLARE_TYPE(RedChannelClient, red_channel_client, RED_CHANNEL_CLIENT);
+typedef struct RedChannel RedChannel;
typedef struct RedClient RedClient;
typedef struct IncomingHandler IncomingHandler;
-typedef struct RedChannel RedChannel;
-typedef struct RedChannelClient RedChannelClient;
-typedef struct RedChannelClientClass RedChannelClientClass;
-typedef struct RedChannelClientPrivate RedChannelClientPrivate;
-
-GType red_channel_client_get_type(void) G_GNUC_CONST;
-
/*
* When an error occurs over a channel, we treat it as a warning
* for spice-server and shutdown the channel.
obviously the big macro should be defined in a separate header and reused.
It basically defines the mainly boilerplate using 1 line instead of 10.
I think the main reason for not using a technique like this in GObject
directly is portability. This require a compiler supporting "static inline"
which is C99. Looks like should not a problem in 2016 but I really had
complaints (this year) when I started using some C99 feature on a portable
project. But we rely on many feature that clang/gcc provides and these
languages supported "static inline" much earlier then C99 (and we also
use "static inline" already).
>
> > > > >
> > > > > 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.
>
> In case it wasn't clear, I was trying to suggest that I will remove
> this extra code. I simply missed it.
>
> Jonathon
>
Frediano
More information about the Spice-devel
mailing list