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

[Jonathon Jongsma]

On Fri, 2015-11-20 at 22:22 +0100, David Jaša wrote:
> On Pá, 2015-11-20 at 16:26 +0100, Francois Gouget wrote:
> > 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?
> > 
> 
> Without accompanying modification, yes. Users are taught that
> spice-server logging is controlled by SPICE_DEBUG_LEVEL environment
> variable. Glib debugging is controlled differently so if you just start
> mixing spice_* and g_* logging functions, users will start silently
> losing some debugging information...

I don't think any debugging information will be lost. The g_return_* functions
log a message with "critical" severity. And critical warnings are printed by
default. It's true that the spice_return_* and g_return_* messages will be
controlled by two different mechanisms, but by default I believe these messages
will be visible. Of course, we should eventually standardize on a single
approach (and provide compatibility with the old settings), but I don't think
incremental changes will be a big problem.


> 
> So from users' perspective, switch to glib logging style should either
> be atomic & properly announced, or gradual with something that would
> turn on glib logging according to SPICE_DEBUG_LEVEL setting.
> 
> Note that client logging is not relevant here as spice-gtk follows glib
> style since its inception, unlike spice-server.
> 
> David
> 
> > 
> > > 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?
> > 
> > 
> 
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel