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

[Fabiano Fidêncio]

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

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


> Frediano


Best Regards,
-- 
Fabiano Fidêncio