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

[Christophe Fergeau]

On Thu, Aug 24, 2017 at 05:11:34AM -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.

I would have gone with something more standard,

if test "x$enable_extra_checks" = xyes; then
  AC_DEFINE_UNQUOTED([ENABLE_EXTRA_CHECKS], 
                     [1],
                     [Define to 1 to enable extra checks on code otherwise defined to 0])
fi

and then

#ifdef ENABLE_EXTRA_CHECKS
static int enable_extra_checks = 1;
#else
static int enable_extra_checks = 0;
#endif

if (enable_extra_checks) {
}

Christophe