[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

Forget... another think weird is that I did not detect a leak.
I'll do some test.

Frediano