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

[Christophe de Dinechin]

> On 20 Mar 2018, at 12:07, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>> 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.

Not sure who “we” is, I was not in it, so I was unaware of that decision process. But good, if that’s for consistency with project X and Y, add this rationale to the commit log.

> I think extra is better, not only expensive but also paranoid tests
> I would say that you don't want on final product.

That is, unfortunately, not what the commit log says. It says “expensive or additional”. So now I’m really confused to what “extra” is supposed to stand for.

“extra” has three meanings according to the dictionary:
	• More than or beyond what is usual, normal, expected, or necessary. See Synonyms at superfluous.
	• Better than ordinary; superior:  extra fineness. 
	• Subject to an additional charge:  Coffee does not come with dinner but is extra.

I assume we don’t mean “superfluous”, and I can’t see how that can be “superior”. So I assume that this means these tests cost extra, they have an additional cost. If so, it’s clearer as “expensive”.

The commit log should help me figure out what tests fall under the “extra” category. Right now, the only reason I am aware of why a test should become “extra” is that it’s too expensive to be left in the code by default. Do you have another rationale in mind?

I’m also confused by the use of this option in another patch to do a premature optimization eliminating spice_assert checks that are quite inexpensive. So that first example you gave contradicts the “expensive” rationale.

The rationale about “higher level libraries” in your commit log seems to indicate that the intent is for this to be enabled when building spice-common and disabled when building the client and server libraries, which IMO adds to the confusion.

> 
>>> 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 thus said the Lord”, said Frediano :-)

But I’m fine with “extra” as long as the commit log correctly explains what “extra” means. Also update the style guide to explain how we use it.


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

Because your commit log says “additional and expensive”, and that the first patch using this “extra check” disables tests that anything but 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.

“I prefer a core than a remote code execution” means that you think we should leave the tests enabled by default, and I agree. So the rationale for disabling a test should be quite clear. To me, the rationale “too expensive to be left on by default” is clear enough. The rationale “They are extra, not expensive” is not.

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