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

[Frediano Ziglio]

> On Thu, Oct 22, 2015 at 3:51 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>
> >> > 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.
> 
> Indeed.
> 

Removed

Frediano