[Spice-devel] spice-server, logging and style

[Frediano Ziglio]

> 
> Hey,
> 
> So there has been quite some discussions about logging.
> 
> To sum up my personal thoughts on this:
> 
> - we should just switch to using glib for logging. This means some mass
>   renaming will need to be done at some point. Maybe it's possible to do
>   it on the whole refactoring branch through git filter-branch or some
>   similar tool, if not we can keep some #define spice_warning g_warning
>   for now. We can also deprecate SPICE_DEBUG_LEVEL/SPICE_ASSERT_LEVEL
>   while still keeping them kind of working with glib.
> 
> - spice_return_if_fail needs to be dealt with though, either we decide
>   it's fine to have it return instead of asserting, and we can just
>   change it to g_return_if_fail, either we decide it's not safe, and its
>   name should more accurately reflect that it asserts rather than
>   returning. I would not change it to g_assert() to make it easy to spot
>   the places we need to review whether we can g_return_if_fail() or not.
> 
> - when a commit from the refactoring branch changes an assert() to a
>   spice_return_if_fail(), we need to have a proper justification as to
>   why this is safe to do, and also use g_return_if_fail() rather than
>   the spice version which asserts. If the patch looks safe, I would not
>   reject it on the basis that "we don't want to look at spice_assert()
>   changes now". When these changes are reviewed now, this means less
>   things to audit later.
> 
> 
> Regarding use of assert() VS g_return_if_fail().

I think you mean spice_assert here. Unfortunately all these macro are slightly
different

> glib documentation for g_return_if_fail() says « If expr evaluates to
> FALSE, the current function should be considered to have undefined
> behaviour (a programmer error). The only correct solution to such an
> error is to change the module that is calling the current function, so
> that it avoids this incorrect call. »
> 
> In short, I'd use it to document programmer expectation/assumption. The
> most classical use is to validate the parameters passed to (usually
> non-static) methods. Imo it's useful to do this in more methods than
> just the spice-server/QEMU interface, but also for most non-static methods.
> Then you can also document some more assumption, like "at this point,
> when I call this method, its return value really should not be NULL".
> If we know nothing bad will happen by returning early, it's better to
> just do it rather than dying in an assert.
> 
> Then, there are some cases where there is no real good way to deal with
> the unexpected situation, even though it's bad, an assert is still
> better than getting in an unpredictable situation (potentially
> crashes/...).
> 
> Regarding inconsistencies detected in refcounts, I don't have a strong
> opinion on assert'ing VS not assert'ing, but since it can indicated memory
> corruption, assert'ing makes sense. We can always change it if this
> causes too much trouble.
> 
> 
> I'll look some more at changing spice-common/common/log.[ch] to use glib
> instead of its own logging, and I'll see how we can make a global search
> and replace on the branch which is currently being merged if there is no
> strong disagreement to all of this.
> 
> Christophe
> 

assert (the base C) abort on condition failed and can be compiled out. Somebody
by default disable these checks on release (I call it the Windows style) while
other prefer to never disable (I call the Unix style). If disabled they are no
op.

spice_assert (spice) abort on a condition failed and cannot be disabled.

g_return_if_* (glib) return on a condition failed and cannot be disabled. If
disabled they are no-op.


I think returning vs abortion here is a bit of a personal opinion. We should
decide (kind polling) on what to do and stick to it. I personally prefer
the abort if should be 100% true.
This does not mean I'm suggesting to go all aborts or all returns, handling
condition could mean to do stuff differently too.

Frediano