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

[Francois Gouget]

On Thu, 19 Nov 2015, Frediano Ziglio wrote:
[...]
> > What do you mean by "100% compatible with the current code"? (why is
> > g_return_if-fail() not "100% compatible with the current code" ?)
> 
> well... implementation is quite different. I didn't get all differences but
> - spice_logv use some environment SPICE_* specific (I doubt Glib does!);
> - Glib output on standard error or output based on level;
> - surely something I forgot!

Does it matter?
The client uses the g_return_xxx() functions already. Would it be a 
problem if the server did too instead of going its separate way?


> Didn't investigate so deep to be able to tell all list but surely just 
> with these I'm not comfortable to do a sed and release tomorrow...

Let me summarize. Currently we have:

1. Plain old and trustworthy assert().
   Used by server/dispatcher.c.

2. g_return_if_fail() which does not log the way spice functions do.
   Used by server/reds_stream.c and most of the client code.

3. spice_assert() which crashes the application.
   Used by most of the server code but not by the client.

4. spice_return_if_fail() which claims to return but instead crashes the 
   application, exactly like spice_assert().
   Also used in most of the server code but not by the client.

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.

Not fixing spice_return_if_fail() does not make any sense either (yes, a 
functions that crashes the application under the guise of returning to 
the caller is *totally* buggy).

The code calling spice_return_if_fail() was written under the assumption 
that it would return. So just switch the default 
SPICE_ABORT_LEVEL_DEFAULT to SPICE_LOG_LEVEL_ERROR.

But it would also really help if the different Spice pieces like the 
server and client could agree on whether to use glib functions or not.


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.



Then there's memory handling where we see the same issues yet again:

1. malloc() & co.
   Used in server/red_replay_qxl.c and some lz encoders.

2. spice_malloc() & co.
   Used by most of the server code but not by the client.

3. g_malloc() & co.
   Use by most of spice-gtk and but only a test file on the server!


This duplication of basic functionality needs to stop. It's confusing 
and can only lead to bugs. Nobody wants to track for each pointer 
whether it should be freed with free(), g_free() or the nonexistent 
spice_free()!

 
> Calling spice_* functions instead of spice_* functions looks like 100% compatible :)

I'm not very impressed by this argument. Do you mean that, just because 
they both start with 'spice_', spice_assert() and spice_strndup() are 
interchangeable?


-- 
Francois Gouget <fgouget at codeweavers.com>