[Spice-devel] [PATCH spice-server] reds: Free remaining configuration

[Frediano Ziglio]

> 
> On Wed, Mar 01, 2017 at 09:57:29AM +0000, Frediano Ziglio wrote:
> > Free security, migration, sasl and name stuff.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/reds.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index 898be92..eabb63f 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -3640,6 +3640,24 @@ SPICE_GNUC_VISIBLE int spice_server_init(SpiceServer
> > *reds, SpiceCoreInterface *
> >      return ret;
> >  }
> >  
> > +static void reds_config_free(SpiceServer *reds)
> 
> It's called reds_config_free, so I'd expect to see a RedServerConfig
> argument (you can set reds->config to NULL in the caller).
> 
> > +{
> > +    reds_mig_release(reds);

yes, this is the reason for the weird argument... I'll try
to change also reds_mig_release.

> > +    ChannelSecurityOptions *now = reds->config->channels_security, *next;
> 
> I'd rather avoid 2 declarations on the same line, especially if one
> comes with initialization. And I'd keep the variable declarations at the
> top. And let's not introduce new use of 'now', imo it's a very bad name
> for an iterator/the current item (feel free to ignore this last comment
> if you disagree).
> 

Usually I use "curr", maybe was some inspiration for some
other code I was looking at that time... I prefer curr, I'll use it.

> Do we have a test case showing at least part of these as leaks?
> 

Was thinking to add a unit test. Our tests don't set these stuff.

> Patch looks good to me.
> 
> Christophe
> 
> > +    for (; now; now = next) {
> > +        next = now->next;
> > +        free(now);
> > +    }
> > +#if HAVE_SASL
> > +    free(reds->config->sasl_appname);
> > +#endif
> > +    free(reds->config->spice_name);
> > +    g_array_unref(reds->config->renderers);
> > +    g_array_unref(reds->config->video_codecs);
> > +    free(reds->config);
> > +    reds->config = NULL;
> > +}
> > +
> >  SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer *reds)
> >  {
> >      /* remove the server from the list of servers so that we don't attempt
> >      to
> > @@ -3668,10 +3686,7 @@ SPICE_GNUC_VISIBLE void
> > spice_server_destroy(SpiceServer *reds)
> >      stat_file_free(reds->stat_file);
> >  #endif
> >  
> > -    g_array_unref(reds->config->renderers);
> > -    g_array_unref(reds->config->video_codecs);
> > -    free(reds->config);
> > -
> > +    reds_config_free(reds);
> >      free(reds);
> >  }
> >  

Frediano