[Spice-devel] [PATCH] log: add not fatal spice_return function

[Francois Gouget]

On Fri, 20 Nov 2015, Christophe Fergeau wrote:
[...]
> > 5. And you propose adding spice_return_if_fail_warning() to fix this mess.
> > 
> > I really don't see how adding more functions is going to make this less 
> > confusing and error prone! Particularly if there is not a concerted 
> > and swift effort to clean up the old code.
> 
> If we were to rename spice_return_if_fail() to spice_assert_if_fail() or
> such, a global replace and a git rebase -x "sed -i
> s/spice_return_if_fail()/spice_assert_if_fail()/gc" would indeed be in
> order.

If not doing a straight replace with g_return_if_fail() I think 
this would be a good first step and clarify the code.


> > Note that spice_error() needs to be fixed too. That name implies the 
> > function logs an error just like spice_warning() logs a warning, not 
> > that it crashes the application. spice_error() should be renamed to 
> > spice_fatal(). For consistency it might make sense to rename 
> > SPICE_LOG_LEVEL_ERROR to SPICE_LOG_LEVEL_FATAL.
> 
> spice_error() is consistent with g_error() here:
> https://developer.gnome.org/glib/2.46/glib-Message-Logging.html#g-error
> "Error messages are always fatal, resulting in a call to abort() to
> terminate the application. This function will result in a core dump;
> don't use it for errors you expect. Using this function indicates a bug
> in your program, i.e. an assertion failure."

Following the glib convention does make sense. I do disagree with glib's 
naming choice though but there's nothing that can be done about that at 
this point.


[...]
> NB: most of the time g_free() and free() are equivalent (they always are
> with newer glib). Very not clean to mix and match g_malloc/free, but
> nothing terribly wrong should happen either.

That may be true right now but does glib explicitly condone it? As long 
as you use g_alloc()+g_free() they can change their implementation as 
they want and your code will still work.

That's what bothers me with spice_malloc(): such pointers can only be be 
freed with plain free(). It hardcodes the assumption that spice_malloc() 
is a wrapper around plain malloc() everywhere in the code, making it 
impossible to switch to some other allocator. At least not until every 
user of spice-common has been fixed.


-- 
Francois Gouget <fgouget at codeweavers.com>