[Spice-devel] [PATCH v4 04/12] Rephrase assertion section

[Christophe Fergeau]

On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin at redhat.com>
> 
> Changes since v3:
> - Integrate comments about performance impact and solution
> - Integrate comments about impact of a failed assertion
> - Add prohibition against side effects
> 
> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
>  docs/spice_style.txt | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 5dc41cb1..3bc70570 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -113,10 +113,40 @@ FIXME and TODO
>  
>  Comments that are prefixed with `FIXME` describe a bug that need to be fixed. Generally, it is not allowed to commit new code having `FIXME` comment. Committing  `FIXME` is allowed only for existing bugs. Comments that are prefixed with `TODO` describe further features, optimization or code improvements, and they are allowed to be committed along with the relevant code.
>  
> -ASSERT
> -------
> +Assertions
> +----------
> +
> +Assertions help testing function arguments and function results validity. As a result, they make it easier to detect bugs. Also, by expressing the intent of the developer, they make the code easier to read.
> +
> +Assertions checks have a significant performance impact should be placed under the `ENABLE_EXTRA_CHECKS` conditional.

"which have" ?

> +
> +[source,c]
> +----
> +void function(void)
> +{
> +    while (condition()) { // Hot loop
> +        spice_assert(ENABLE_EXTRA_CHECKS && expensive_check());
> +    }
> +    ...
> +}
> +----
> +
> +Several form of assertion exist. SPICE does not use the language `assert` much, but instead relies on the following variants:
> +
> +- `spice_assert`, defined in `common/log.h`, is never disabled at run-time and prints an error if the assertion fails.
> +
> +- `g_assert` and other variants defined in glib such as `g_assert_null`, can be disabled by setting `G_DISABLE_ASSERT` (which is never done in practice for SPICE code), and emits a message through the g_assertion_message function and its variants.

I would expect program abortion to be mentioned here in addition to the
printing of the message.

> +The guidelines for selecting one or the other are not very clear from
> the existing code. The `spice_assert` variant may be preferred for
> SPICE-only code, as it makes it clearer that this assertion is coming
> from SPICE. The `g_assert` and variants may be preferred for
> assertions related to the use of glib.

I'd remove this and just say to use g_*

> +
> +Be mindful of the impact of a failing assertion. For example, assertions in the server should not abort it, as this would kill the QEMU process and the virtual machine.
> +
> +Assertions should not:
> +
> +- Be used as a replacement for proper error management or to check unsafe data such as user input
> +- Have side effects, since an assertion check may be disabled

*if* we really plan to disable assertions in some cases, I think we'd
need to be more explicit about when it's going to happen. If we don't
consider it a valid usecase to disable assertions, then we can drop
this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180216/3c315a93/attachment.sig>