[Spice-devel] [PATCH spice-common 1/2] Add --enable-extra-checks option

[Frediano Ziglio]

> > On 19 Mar 2018, at 14:46, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> > Allow to enable code to do additional or expensive checks.
> 
> “Allow to enable…” -> “Add configuration option enabling expensive checks”
> 

We decided extra after researching on different projects.
I think extra is better, not only expensive but also paranoid tests
I would say that you don't want on final product.

> > The option should be used by higher level libraries.
> > By default the option is disabled.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> > common/log.h     |  6 ++++++
> > configure.ac     |  1 +
> > m4/spice-deps.m4 | 14 ++++++++++++++
> > 3 files changed, 21 insertions(+)
> > 
> > diff --git a/common/log.h b/common/log.h
> > index 06d48d2..e0fd34b 100644
> > --- a/common/log.h
> > +++ b/common/log.h
> > @@ -93,6 +93,12 @@ void spice_log(GLogLevelFlags log_level,
> >     }                                                   \
> > } G_STMT_END
> > 
> > +#if ENABLE_EXTRA_CHECKS
> > +enum { spice_extra_checks = 1 };
> 
> I would suggest renaming “extra_checks” to “expensive_checks” if that’s
> really the intent.

No, is extra.

> And then, I would avoid bracketing inexpensive checks (like simple asserts)
> with it, because otherwise it introduces confusion.
> 

I don't see why an if is confusing. They are "extra", not "expensive".

> Alternatively, if we think that asserts are expensive, then all spice_assert
> checks should be disabled by this. But I’d rather not.
> 
> (I already suggested a spice_hot_assert for assertions in hot code as a
> better way to express the intent than an if around spice_assert).
> 

I don't agree on a rule like "is expensive disable the assert", depends
on the probability of happening and the problem that can raise not doing
it. I prefer a core than a remote code execution, but here we enter on
the opinions.

> > +#else
> > +enum { spice_extra_checks = 0 };
> > +#endif
> > +
> > SPICE_END_DECLS
> > 
> > #endif /* H_SPICE_LOG */
> > diff --git a/configure.ac b/configure.ac
> > index 3542161..3da85de 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -27,6 +27,7 @@ fi
> > AM_PROG_CC_C_O
> > 
> > SPICE_CHECK_SYSDEPS
> > +SPICE_EXTRA_CHECKS
> > 
> > AC_ARG_ENABLE([alignment-checks],
> >   AS_HELP_STRING([--enable-alignment-checks],
> > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > index 68e3091..a6f4b7b 100644
> > --- a/m4/spice-deps.m4
> > +++ b/m4/spice-deps.m4
> > @@ -23,6 +23,20 @@ AC_DEFUN([SPICE_PRINT_MESSAGES],[
> > ])
> > 
> > 
> > +# SPICE_EXTRA_CHECKS()
> > +# --------------------
> > +# Check for --enable-extra-checks option
> > +# --------------------
> > +AC_DEFUN([SPICE_EXTRA_CHECKS],[
> > +AC_ARG_ENABLE([extra-checks],
> > +               AS_HELP_STRING([--enable-extra-checks=@<:@yes/no@:>@],
> > +                              [Enable expensive checks
> > @<:@default=no@:>@]))
> > +AM_CONDITIONAL(ENABLE_EXTRA_CHECKS, test "x$enable_extra_checks" = "xyes")
> > +AS_IF([test "x$enable_extra_checks" = "xyes"],
> > +      [AC_DEFINE([ENABLE_EXTRA_CHECKS], 1, [Enable extra checks on
> > code])])
> > +])
> > +
> > +
> > # SPICE_CHECK_SYSDEPS()
> > # ---------------------
> > # Checks for header files and library functions needed by spice-common.

Here comments would be better using "dnl" style but I used shell comments
for coherence.

Frediano