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

[Fabiano Fidêncio]

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.

>>
>> 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);

-- 
Fabiano Fidêncio