>
> On Wed, Oct 21, 2015 at 08:37:25AM -0400, Frediano Ziglio wrote:
> >
> > >
> > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> > >
> > > ---
> > > 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().
>
> 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.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel