[Spice-devel] [PATCH 06/19] Convert Dispatcher and MainDispatcher to GObjects

[Frediano Ziglio]

> 
> On Fri, 2016-02-19 at 06:54 -0500, Frediano Ziglio wrote:
> > > 
> > > From: Jonathon Jongsma <jjongsma at redhat.com>
> > > 
> > > 
> 
> > > -struct MainDispatcher {
> > > -    Dispatcher base;
> > > -    SpiceCoreInterfaceInternal *core;
> > > -    RedsState *reds;
> > > +G_DEFINE_TYPE(MainDispatcher, main_dispatcher, TYPE_DISPATCHER)
> > > +
> > > +#define MAIN_DISPATCHER_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE((o),
> > > TYPE_MAIN_DISPATCHER, MainDispatcherPrivate))
> > > +
> > > +struct MainDispatcherPrivate
> > > +{
> > > +    SpiceCoreInterfaceInternal *core; /* weak */
> > > +    RedsState *reds; /* weak */
> > 
> > Not that weak, try to set them NULL and see what's happening...
> > 
> 
> 
> Yes, perhaps a poor choice of words. My intention was to say that the
> MainDispatcher does not own these objects, and therefore does not have to
> free
> them in the destructor. If they were GObjects, we would probably take a
> reference, but they're not.
> 

"weak pointers" are associated with some idiom where you don't have ownership
of the objects but the pointers does not became invalid. They are used to avoid
circular ownership which is a problem for reference counting. As GObject
implements weak pointers and as we are going to use GObject using weak terminology
on pointers to describe a different semantic make the code confusing.
"If they were GObjects, we would probably take a reference, but they're not" is
wrong. It assumes that without GObjects you cannot implement reference counting.
You can just add a counter to RedsState and a reds_ref+reds_unref.

> 
> 
> > > diff --git a/server/red-dispatcher.c b/server/red-dispatcher.c
> > > index 33cd12f..4b839a9 100644
> > > --- a/server/red-dispatcher.c
> > > +++ b/server/red-dispatcher.c
> > > @@ -48,7 +48,7 @@ struct AsyncCommand {
> > >  struct RedDispatcher {
> > >      QXLWorker base;
> > >      QXLInstance *qxl;
> > > -    Dispatcher dispatcher;
> > > +    Dispatcher *dispatcher;
> > 
> > Here you are changing inheritance to association... (1)
> 
> Yeah, sort of. But I did it for a couple of reasons:
> - RedDispatcher already "inherits" QXLWorker, and inheritance of this sort
> only
> really works if the parent type is the first member within the struct

On some language you can have multiple inheritance, in some other you have
only single inheritance but you can implement multiple "interfaces". Here
QXLWorker is a interface so you can easily decide to implement it.
https://cgit.freedesktop.org/~fziglio/spice-server/commit/?h=dispatcher&id=51c014c58da1916e5c8ac1e2736820d131fec93b
patch is a good start to implement this.
Looking at some refactory patches (like this) looks like we are just using
GObject for the "G" not taking into account the "Object" part of it.
I honestly more concern to the "Object" part!
Surely if we want RedDispatcher to just be a QXLWorker implementation we
should definitely change the name to something like RedQXLWorker!

> - Most of the internals of the Dispatcher struct were moved to an internal
> private struct anyway, so embedding the struct here doesn't really gain you
> much. You can't access that private data directly.
> 

The problem here came from GObject and reference counting. From the
maintenance point of view you want be able to use reference counting on Dispatcher
(which now is a GObject!). However you could have situation where you Dispatcher
is still alive while RedDispatcher is freed. However Dispatcher still point to
RedWorker (and indirectly to RedDispatcher) so you will have a possible
use-after-free. This patch introduce GObject but the objects cannot use reference
counting safely. The OO solution to this is private inheritance however the
Dispatcher is exposed from RedDispatcher (red_dispatcher_get_dispatcher).
If you have to look at the code for this probably you are not applying OO so
you are ignoring the "Object" part of "GObject".

> So in my mind the only real difference is how the memory is allocated: either
> as
> part of the RedDispatcher, or separately in the red_dispatcher_init()
> function.
> Since there's only a very small number of dispatcher objects and they live
> the
> entire life of the the application, that doesn't strike me as a very big
> deal.

So you are telling that RedDispatchers aren't freed (which is true) and
that they won't (be design) never be freed? So why bothering implementing
free code if you are assuming that cannot be called?
With the same idea why bothering freeing RedsState?

> 
> Do you object to this change?
> 

Not base idea of the patch, the implementation just add regressions
for the code quality and maintenance.

> 
> Jonathon
> 
> 

About RedDispatcher name and what should represent. I had a look at
red-dispatcher.[ch] to have a global picture. I was quite scare about
the header. Just execute these commands:

$ grep -ci 'red.\?dispatcher' server/red-dispatcher.h 
22
$ grep -ci 'red.\?worker' server/red-dispatcher.h 
125

so... are we implementing RedWorker or RedDispatcher? From the header
seems more RedWorker!
I don't know the history of these files/structures but (these should be
impartial statements):
- red-dispatcher.c is implementing internal QXLInstance object. Just
  grep for spice_qxl_ function and you'll realize that "only" 100% of
  the functions implementation is there!
- RedDispatcher uses Dispatcher and RedWorker;
- RedWorker implements the working thread to handle messages between
  QXL and Cursor/DisplayChannel;
- red-worker.c registers handlers for Dispatcher;
- in red-dispatcher.c RedWorker* messages are sent using Dispatcher to
  code in red-worker.c. So both files implements QXLInstance;
- RedWorker handles messages from RedDispatcher and commands;
- RedDispatcher and RedWorker are C structures!
Said that and considering RedDispatcher as an object (with
red_dispatcher_* functions being methods of the object)... is it really
a dispatcher? I mean, is implementing the QXLInstance just a "small"
detail?
I was trying to understand who is using RedDispatcher and is not that
clear. Lot of files include red-dispatcher.h for no particular reasons.

Still digging...

Frediano