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@xxxxxxxxxxxxxxx> _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel