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

[Frediano Ziglio]

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

Frediano