[Spice-devel] [PATCH spice-server] Remove core_public and core_interface_adapter globals usage

[Jonathon Jongsma]

On Tue, 2016-11-15 at 11:39 -0500, Frediano Ziglio wrote:
> > 
> > 
> > On Tue, 2016-11-15 at 04:53 -0500, Frediano Ziglio wrote:
> > > 
> > > > 
> > > > 
> > > > 
> > > > On Fri, 2016-11-11 at 12:08 +0000, Frediano Ziglio wrote:
> > > > > 
> > > > > 
> > > > > Avoid not constant globals.
> > > > > 
> > > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > > ---
> > > > >  server/dummy-channel.c          |  5 ++-
> > > > >  server/event-loop.c             | 23 ++++++-----
> > > > >  server/red-channel-client.c     | 32 +++++++--------
> > > > >  server/red-common.h             | 17 ++++----
> > > > >  server/reds-private.h           |  2 +-
> > > > >  server/reds.c                   | 88 ++++++++++++++++++-----
> > > > > ----
> > > > > ----
> > > > > ----------
> > > > >  server/tests/basic_event_loop.c | 37 ++++++++++++++---
> > > > >  7 files changed, 114 insertions(+), 90 deletions(-)
> > > > > 
> > > > > diff --git a/server/dummy-channel.c b/server/dummy-channel.c
> > > > > index f13c8f8..aecd3a6 100644
> > > > > --- a/server/dummy-channel.c
> > > > > +++ b/server/dummy-channel.c
> > > ....
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > >  
> > > > > -    void (*channel_event)(int event, SpiceChannelEventInfo
> > > > > *info);
> > > > > +    void (*channel_event)(const SpiceCoreInterfaceInternal
> > > > > *iface,
> > > > > int event, SpiceChannelEventInfo *info);
> > > > >  
> > > > > -    GMainContext *main_context;
> > > > > +    union {
> > > > > +        GMainContext *main_context;
> > > > > +        SpiceCoreInterface *public_interface;
> > > > > +    };
> > > > >  };
> > > > 
> > > > The last time I reviewed this patch (in June!), I ACKed it, but
> > > > I
> > > > asked
> > > > for a comment here explaining why the main_context and
> > > > public_interface
> > > > were mutually exclusive. Now that it's the future, my
> > > > prediction
> > > > that I
> > > > might find it confusing in the future has come true. I had to
> > > > convince
> > > > myself again. So I'd like to repeat the request for a comment
> > > > again.
> > > > 
> > > > Otherwise it looks fine.
> > > > 
> > > > Jonathon
> > > > 
> > > 
> > > I surely missed.
> > > 
> > > What about:
> > > 
> > > /* Implementation data.
> > >  * Usually these fields are is implemented subclassing the object
> > > and
> > >  * adding new fields, in this case the choices are limited and
> > > both
> > >  * pointers so use an union to store all possible values.
> > >  */
> > > 
> > > Frediano
> > 
> > hmm. That doesn't quite do it for me.  Let me try:
> > 
> > This structure is an adapter that allows us to use the same API to
> > implement the core interface in a couple different ways. The first
> > method is to use a public SpiceCoreInterface provided to us by the
> > library user (for example, qemu). The second method is to implement
> > the
> > core interface functions using the glib event loop. In order to
> > avoid
> > global variables, each method needs to store additional data in
> > this
> > adapter structure. Instead of using a generic void* data parameter,
> > we
> > provide a bit more type-safety by using a union to store the type
> > of
> > data needed by each implementation.
> > 
> > Is that an accurate description?
> > 
> > Jonathon
> > 
> > 
> 
> Sounds good to me.
> Should I send another version or just put this sentence on
> the comment above the union?
> 
> Frediano


If you're happy with that comment as-is, I don't need to see a new
patch.

Jonathon