On Fri, 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? > > > > 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. Note that this is exactly the behavior of g_error, so I don't see any need to change the behavior -- we should just switch to using g_error() instead. > > > > 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel