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

[Christophe Fergeau]

Hey,

On Fri, Nov 20, 2015 at 04:26:22PM +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?

I looked at this today, one different between glib log functions and
SPICE is that the SPICE ones append the file name/line number/function
name to the logged message. glib is not doing that, which means if we
want to keep this, we'd have our own macros calling glib log
functions...
Changing spice_log to call g_log rather than doing the logging itself,
and deprecating SPICE_ABORT_LEVEL/SPICE_DEBUG_LEVEL (while making them
keep working by setting glib abort level/debug level) is quite easy.

But this means we still have spice_* logging functions... Benefit of
that though is that glib messages (from GSplice misuse for example)
and SPICE messages would go through the same channel.
> 
> 
> > 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.

This one could be changed to spice_assert/g_assert

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

Fixed by the aforementioned patch, but if we want to keep line
number/function/... in the logs (and I think we should), then they
sohuld become again spice_something

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

I'm wondering whether we should rename this one spice_assert_if_fail().
Yes it's the same as spice_assert(), but this nicely tags places as
"here a return_if_fail() could make sense, please review".

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

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

I'd love to be able to make that assumption, but I'm not sure it's 100%
true, mainly because of
http://cgit.freedesktop.org/spice/spice-common/commit/?id=c1403ee6bf4dfdd8f614f84ef145083b06a9f23e
which replaces a lot of asserts with return_if_fail(), and which does
not mention at all in the commit log whether all these places were
audited or not.

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

In general, I'd like to see spice-server use more function from glib,
and stops reinventing the wheel. But as I mentioned above, for logging,
it seems it would be best to stick with glib


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


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

I'd migrate away from spice_malloc, but this is far from being a
priority imo.

> 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()!

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.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20151120/7f3a4ebc/attachment.sig>