[Spice-devel] [PATCH spice-server] display-channel: Check that all structure are destroyed during finalize

[Jonathon Jongsma]

On Thu, 2017-08-24 at 05:11 -0400, Frediano Ziglio wrote:
> > 
> > On Tue, 2017-08-22 at 08:54 +0100, Frediano Ziglio wrote:
> > > The leak detector we use currently is not enough to detect
> > > some kind of leak in DisplayChannel so manully test.
> > > These tests are enabled only when --enable-extra-checks is passed
> > > to configure.
> > 
> > 
> > Out of curiosity, did you find a bug that made you add this extra
> > check?
> > 
> 
> No, was more some old code I wrote (I think more than 1 year ago)
> when I started my "cleanup" branch.
> 
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > ---
> > >  configure.ac             |  2 ++
> > >  server/display-channel.c | 22 ++++++++++++++++++++++
> > >  2 files changed, 24 insertions(+)
> > > 
> > > diff --git a/configure.ac b/configure.ac
> > > index e1e74862..7fec66fc 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -247,6 +247,8 @@ AC_ARG_ENABLE([extra-checks],
> > >                 AS_HELP_STRING([--enable-extra-
> > > checks=@<:@yes/no@:>@],
> > >                                [Enable expensive checks
> > > @<:@default=no@:>@]))
> > >  AM_CONDITIONAL(ENABLE_EXTRA_CHECKS, test "$enable_extra_checks"
> > > =
> > > "yes")
> > > +AC_DEFINE_UNQUOTED([ENABLE_EXTRA_CHECKS], [$(test
> > > "x$enable_extra_checks" = xyes && echo 1 || echo 0)],
> > > +                   [Define to 1 to enable extra checks on code])
> 
> A bit of note on this. Usually AC_DEFINEs are not defined or defined
> to 1. This is defined to 0 or 1 which is a bit different from the
> standard. However code like:
> 
>    if (ENABLE_EXTRA_CHECKS)
> 
> cause a compiler error. Maybe the comment "Define to 1 to enable
> extra checks on code" should be more explicit about it?
> On the other way this should affect only people that needs
> to edit manually config.h and as said would be generating a
> compiler error if they undefine that constant.
> A difference from the standard is that code like
> 
> #ifdef ENABLE_EXTRA_CHECKS
> 
> should not be used. Maybe a syntax-check extension could help
> with this.
> 
> On style, maybe the line should be split in 3 lines like
> 
> AC_DEFINE_UNQUOTED([ENABLE_EXTRA_CHECKS], 
>                    [$(test "x$enable_extra_checks" = xyes && echo 1
> || echo 0)],
>                    [Define to 1 to enable extra checks on code
> otherwise defined to 0])
> 
> 
> (this also improves the comment on config.h file)
> 
> > >  
> > >  dnl
> > > =================================================================
> > > ====
> > > ======
> > >  dnl check compiler flags
> > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > index 924d219b..317b02ef 100644
> > > --- a/server/display-channel.c
> > > +++ b/server/display-channel.c
> > > @@ -80,6 +80,28 @@ display_channel_finalize(GObject *object)
> > >  
> > >      display_channel_destroy_surfaces(self);
> > >      image_cache_reset(&self->priv->image_cache);
> > > +
> > > +    if (ENABLE_EXTRA_CHECKS) {
> 
> About this if. Doing it that way instead of the preprocessor
> (#if) way allows the compiler to always check the code instead
> to having to compile the code twice (with and without the
> option).
> The compiler is pretty good at stripping this dead code.
> 
> Looking at the specific code one could object if is worth
> removing that ENABLE_EXTRA_CHECKS check in that path.

yeah, I noticed that this was different than the normal #ifdef, but I
decided that the advantage of having the compiler check the code even
when the extra checks are disabled was probably worth the downsides of
this approach. It's too easy for the code inside #ifdef blocks to decay
and become broken without noticing.



> 
> > > +        unsigned cnt;
> > > +        _Drawable *dr;
> > 
> > I'd really prefer full-name variables: "count", "drawable"
> > 
> > > +        Stream *stream;
> > > +
> > > +        for (cnt = 0, dr = self->priv->free_drawables; dr; dr =
> > > dr-
> > > > u.next) {
> > > 
> > > +            ++cnt;
> > > +        }
> > > +        spice_assert(cnt == NUM_DRAWABLES);
> > > +
> > > +        for (cnt = 0, stream = self->priv->free_streams; stream;
> > > stream = stream->next) {
> > > +            ++cnt;
> > > +        }
> > > +        spice_assert(cnt == NUM_STREAMS);
> > > +        spice_assert(ring_is_empty(&self->priv->streams));
> > > +
> > > +        for (cnt = 0; cnt < NUM_SURFACES; ++cnt) {
> > > +            spice_assert(self->priv-
> > > >surfaces[cnt].context.canvas ==
> > > NULL);
> > > +        }
> > > +    }
> > > +
> > >      monitors_config_unref(self->priv->monitors_config);
> > >      g_array_unref(self->priv->video_codecs);
> > >      g_free(self->priv);
> > 
> > 
> > otherwise OK with me
> > 
> > Acked-by: Jonathon Jongsma <jjongsma at redhat.com>
> > 
> > 
> 
> Frediano