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

[Jonathon Jongsma]

On Mon, 2015-11-23 at 16:20 +0100, Francois Gouget wrote:
> 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.

Yeah, that seems to be a reasonable initial step to me as well.



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