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

[Frediano Ziglio]

> 
> 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.

> > +        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