[Spice-devel] [PATCH 5/9] server: dispatcher_init/dispatcher_new

Thu Oct 22 06:51:09 PDT 2015

> 
> > On Thu, Oct 22, 2015 at 11:48 AM, Frediano Ziglio <fziglio at redhat.com>
> > wrote:
> > >>
> > >> On Wed, Oct 21, 2015 at 3:53 PM, Frediano Ziglio <fziglio at redhat.com>
> > >> wrote:
> > >> >>
> > >> >> On Wed, Oct 21, 2015 at 08:37:25AM -0400, Frediano Ziglio wrote:
> > >> >> >
> > >> >> > >
> > >> >> > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > >> >> > >
> > >> >> > > ---
> > >> >> > >  server/red_dispatcher.c | 6 ++++--
> > >> >> > >  server/red_dispatcher.h | 2 +-
> > >> >> > >  server/reds.c           | 2 +-
> > >> >> > >  3 files changed, 6 insertions(+), 4 deletions(-)
> > >> >> > >
> > >> >> > > diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> > >> >> > > index 0bc853d..c43da7d 100644
> > >> >> > > --- a/server/red_dispatcher.c
> > >> >> > > +++ b/server/red_dispatcher.c
> > >> >> > > @@ -1060,7 +1060,7 @@ static RedChannel
> > >> >> > > *red_dispatcher_cursor_channel_create(RedDispatcher *dispatche
> > >> >> > >      return cursor_channel;
> > >> >> > >  }
> > >> >> > >
> > >> >> > > -void red_dispatcher_init(QXLInstance *qxl)
> > >> >> > > +RedDispatcher *red_dispatcher_new(QXLInstance *qxl)
> > >> >> > >  {
> > >> >> > >      RedDispatcher *red_dispatcher;
> > >> >> > >      WorkerInitData init_data;
> > >> >> > > @@ -1069,7 +1069,7 @@ void red_dispatcher_init(QXLInstance *qxl)
> > >> >> > >      RedChannel *cursor_channel;
> > >> >> > >      ClientCbs client_cbs = { NULL, };
> > >> >> > >
> > >> >> > > -    spice_return_if_fail(qxl->st->dispatcher == NULL);
> > >> >> > > +    spice_return_val_if_fail(qxl->st->dispatcher == NULL, NULL);
> > >> >> > >
> > >> >> >
> > >> >> > This is just going to leak the old dispatcher if already set, see
> > >> >> > below.
> > >> >> > This should be an assert.
> > >> >>
> > >> >> If spice_return_val_if_fail() are anything like
> > >> >> g_return_val_if_fail(),
> > >> >> they usually mean "programming error, anything may happen from this
> > >> >> point". If there's only a minor leak when this occurs, this is fair
> > >> >> game
> > >> >> imo, and better than an assert().
> > >>
> > >> I agree with Christophe here.
> > >>
> > >
> > > Actually the old global behavior was "if dispatcher was already
> > > initialized
> > > do
> > > nothing" the actual one (after the patch) is "if dispatches was already
> > > initialized set the pointer to NULL and leak it", so when you will access
> > > the
> > > pointer you will get a core dump. Honestly I think the old one was
> > > better.
> > > Actually this can't never happen as the pointer is always NULL at that
> > > check.
> > >
> > >> >>
> > >> >> Christophe
> > >> >>
> > >> >
> > >> > Usually I like to think about contracts
> > >> >
> > >> >   void red_dispatcher_init(QXLInstance *qxl)
> > >> >
> > >> > says "initialize a dispatcher given a QXLInstance object" while
> > >> >
> > >> >   RedDispatcher *red_dispatcher_new(QXLInstance *qxl)
> > >> >
> > >> > says "create a new dispatcher given this QXLInstance object".
> > >> >
> > >> > With first contract the check make more sense while in the last one
> > >> > one
> > >> > could argue that the function should just create a new object. The
> > >> > check
> > >> > assume that there will be a relationship between the instance qxl and
> > >> > the
> > >> > created dispatcher which is made clear in the caller setting
> > >> > qxl->st->dispatcher
> > >> > so why should not be this assignment inside red_dispatcher_new if they
> > >> > both
> > >> > have this knowledge?
> > >> >
> > >> > This assume a 1-to-1 relationship between the dispatcher and the
> > >> > worker
> > >> > which for me would prefer a red_dispatcher_init than a
> > >> > red_dispatcher_new.
> > >>
> > >> I am not sure if I understand your point here. Frediano.
> > >>
> > >> For a cleaner code, red_dispatcher_new() must just create a dispatcher
> > >> given the QXLInstance object, but I would prefer to set
> > >> qxl->st->dispatcher out of this function.
> > >> I mean, having something like: qxl->st->dispatcher =
> > >> red_dispatcher_new(qxl);
> > >>
> > >
> > > Yes, new patch add a line like this. Actually there is this line and also
> > > qxl->st->dispatcher is set inside red_dispatcher_new.
> > > I think that if the function is called red_dispatcher_new is a caller
> > > responsibility
> > > to check that qxl->st->dispatcher is NULL before calling it to avoid the
> > > leak and
> > > also to set qxl->st->dispatcher with the value returned by
> > > red_dispatcher_new.
> > >
> > 
> > Yeah, in the end I agree with your point.
> > 
> > 
> > > So in the end I would:
> > > - remove qxl->st->dispatcher == NULL check inside red_dispatcher_new;
> > 
> > Agreed.
> > 
> > > - remove qxl->st->dispatcher set inside red_dispatcher_new;
> > 
> > Agreed
> >
> 
> I tried to do these changes. Unfortunately after this setting
> red_dispatcher_new
> calls some callbacks which require qxl->st->dispatcher to be set,
> specifically
> 
>     qxl->st->qif->attache_worker(qxl, &red_dispatcher->base);
>     qxl->st->qif->set_compression_level(qxl, calc_compression_level());
> 
> I would suggest either
> - move these line in the caller;
> - ditch the patch.
>  

Tried to move the lines in the caller (spice_server_add_interface in server/reds.c),
they called a call_compression_level function which is defined in red_dispatcher.c.
Made the function not static and exported but one of the lines access qxl->st->dispatcher
which is not defined entirely.

> > > - assure qxl->st->dispatcher == NULL before calling red_dispatcher_new;
> > 
> > Agreed. And from the only place where the function is called, it is.
> > (two line above the qxl->st->dispatcher = red_dispatcher_new() you
> > have a qxl->st = spice_new0(QXLState, 1),
> > 
> > > - assume red_dispatcher_new never returns NULL (as currently does).
> > >
> > 
> > I wouldn't assume that. I would assume red_dispacther_new() returns
> > NULL in case of error.
> > 
> 
> Actually this is derived from spice_new0 never returning NULL and missing
> handling of failures in red_dispacther_new.
> 

After all these considerations I think would be much more reasonable to
remove entirely the patch.

I was trying to understand how these objects are related to each other
and why these initialization are so convoluted.
I draw a diagram (https://drive.google.com/file/d/0B-Yz2R8Rod-AQU03azZBR1Z4S0E/view?usp=sharing)
so see the relationship between different objects.
QXLState has just two fields (qif and dispatcher) and is mainly used to bind
QXLInstance and RedDispatcher. The qif field is used mainly (only once in reds.c) in
red_worker.c and red_dispatcher.c. Usually you start with a pointer to QXLInstance so
the path to discover QXLInterface is qxl->st->qif while using qxl->base.sif
(converting it to QXLInterface) is faster.
IMHO RedDispatcher is the private implementation of a QXLInstance object hidden
but all these pointers. For instance RedDispatcher build and manage the worker.

Frediano