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

[Fabiano Fidêncio]

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.

>
> I was trying to understand how these objects are related to each other
> and why these initialization are so convoluted.
> I draw a diagram (https://drive.google.com/file/d/0B-Yz2R8Rod-AQU03azZBR1Z4S0E/view?usp=sharing)
> so see the relationship between different objects.
> QXLState has just two fields (qif and dispatcher) and is mainly used to bind
> QXLInstance and RedDispatcher. The qif field is used mainly (only once in reds.c) in
> red_worker.c and red_dispatcher.c. Usually you start with a pointer to QXLInstance so
> the path to discover QXLInterface is qxl->st->qif while using qxl->base.sif
> (converting it to QXLInterface) is faster.
> IMHO RedDispatcher is the private implementation of a QXLInstance object hidden
> but all these pointers. For instance RedDispatcher build and manage the worker.
>
> Frediano



-- 
Fabiano Fidêncio