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

[Frediano Ziglio]

> 
> 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.

So in the end I would:
- remove qxl->st->dispatcher == NULL check inside red_dispatcher_new;
- remove qxl->st->dispatcher set inside red_dispatcher_new;
- assure qxl->st->dispatcher == NULL before calling red_dispatcher_new;
- assume red_dispatcher_new never returns NULL (as currently does).

Frediano