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

[Frediano Ziglio]

> Hi,
> 
> On Fri, Nov 20, 2015 at 01:12:27PM -0500, Frediano Ziglio wrote:
> > 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.
> 
> I don't think we want to be able to disable assertions anyway, when they
> are here it's because something bad would happen otherwise :)
> 

That is the g_return_* macros we want to use are NOT following GLib behavior.
Not of a problem but this should be documented in the style guide.

Is not really a small difference this assumption!

> > g_return_if_* (glib) return on a condition failed and cannot be disabled.
> > If
> > disabled they are no-op.
> 
> There is G_DISABLE_CHECKS to turn them into 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.
> 
> Maybe you are suggesting more or less the same thing :).
> To me, from worst to "less worse", when something unexpected happens:
> - not detected, code continues running but behaves unpredictably (can
>   easily lead to a security vulnerability if this can be triggered from
>   the guest)
> - detect the condition, and abort (assert())
> - detect the condition, log it, and keep running (return_if_fail())
> 

In some condition point 3 can be the same at point 1 so the order is a
bit scary to me. The return create two paths (taken or not) which
should be considered. The spice_assert has only one path; the condition
is met!

> asserting is more comfortable for us developers, and probably easier,
> but this also means we are killing a user VM, so this should not be done
> lightly, which is why return_if_fail() is vastly better.
> It's probably not always possible to easily deal gracefully with such
> conditions, so yes, assert() is still an option when we don't have
> better choices.
> 
> Christophe
> 

Well, what's worst than killing a VM? Leaving the host die because we
are too lazy!

Frediano