[Spice-devel] [PATCH spice-server] worker: Free encoders when client disconnects

[Frediano Ziglio]

> On Fri, 2015-11-20 at 10:50 +0100, Fabiano Fidêncio wrote:
> > On Fri, Nov 20, 2015 at 10:08 AM, Pavel Grunt <pgrunt at redhat.com> wrote:
> > > Signed-off-by: Pavel Grunt <pgrunt at redhat.com>
> > > ---
> > > Seems that free functions haven't been used since there were introduced
> > > [1]
> > > 
> > > [1]
> > > http://lists.freedesktop.org/archives/spice-devel/2015-November/023893.h
> > > tml
> > > ---
> > >  server/dcc-encoders.c | 11 +++++++++++
> > >  server/dcc-encoders.h |  1 +
> > >  server/red_worker.c   |  1 +
> > >  3 files changed, 13 insertions(+)
> > > 
> > > diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> > > index 90d0ce0..6b764ba 100644
> > > --- a/server/dcc-encoders.c
> > > +++ b/server/dcc-encoders.c
> > > @@ -406,6 +406,17 @@ void dcc_encoders_init(DisplayChannelClient *dcc)
> > >      dcc->zlib_level = ZLIB_DEFAULT_COMPRESSION_LEVEL;
> > >  }
> > > 
> > > +void dcc_encoders_free(DisplayChannelClient *dcc)
> > > +{
> > > +    quic_destroy(dcc->quic);
> > > +    lz_destroy(dcc->lz);
> > > +    jpeg_encoder_destroy(dcc->jpeg);
> > > +#ifdef USE_LZ4
> > > +    lz4_encoder_destroy(dcc->lz4);
> > > +#endif
> > > +    zlib_encoder_destroy(dcc->zlib);
> > > +}
> > > +
> > >  static void marshaller_compress_buf_free(uint8_t *data, void *opaque)
> > >  {
> > >      compress_buf_free((RedCompressBuf *) opaque);
> > > diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> > > index cdb44ac..30f54d5 100644
> > > --- a/server/dcc-encoders.h
> > > +++ b/server/dcc-encoders.h
> > > @@ -36,6 +36,7 @@ typedef struct RedCompressBuf RedCompressBuf;
> > >  typedef struct GlzDrawableInstanceItem GlzDrawableInstanceItem;
> > > 
> > >  void             dcc_encoders_init
> > >                             (DisplayChanne
> > > lClient *dcc);
> > > +void             dcc_encoders_free
> > >                            (DisplayChanne
> > > lClient *dcc);
> > >  void             dcc_free_glz_drawable_instance
> > >                (DisplayChanne
> > > lClient *dcc,
> > >                                                                GlzDrawableIn
> > > stanceItem *item);
> > >  void             marshaller_add_compressed
> > >                     (SpiceMarshall
> > > er *m,
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 32612d5..cca7f8c 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -5360,6 +5360,7 @@ static void
> > > display_channel_client_on_disconnect(RedChannelClient *rcc)
> > >      free(dcc->send_data.stream_outbuf);
> > >      free(dcc->send_data.free_list.res);
> > >      dcc_destroy_stream_agents(dcc);
> > > +    dcc_encoders_free(dcc);
> > > 
> > >      // this was the last channel client
> > >      spice_debug("#draw=%d, #red_draw=%d, #glz_draw=%d",
> > > --
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > 
> > Nice catch, ACK.
> 
> Pushed, thanks
> 
> Pavel
> 

I don't know but I feel some itch... could you set pointers to NULL
after the _destroy ?
It's quite strange as the destruction is not actually perfectly
symmetric. On dcc_new the encoders are created but freed on disconnect.
This lead the paths from on_disconnect to red_channel_client_unref (which
are actually a lot) with dandling pointers.

I'm missing a virtual destructor here...

Frediano