[Spice-devel] spice-server, logging and style

[Marc-André Lureau]

Hi

----- Original Message -----
> Hi,
>   looking at refactory branch and spice-server code looks like logging
> facilities are used all around the code for different purposes however
> don't looks like there are some specific style or direction to handle
> these stuff.
> At http://cgit.freedesktop.org/spice/spice/tree/docs there is a
> Spice_style.odt file but the only reference to logging is the ASSERT
> macro which is only used in some spice-server test.
> 
> To sum up the state of logging:
> - the main logging macro/functions are in spice-common/common/log.h
>   header;
> - there are 5 levels (debug, info, warning, critical, error)
>   following GLib levels (error is fatal);
> - there are generic function (spice_log, spice_logv);
> - there are function to log at the different log levels (spice_debug,
>   spice_info, spice_warning, spice_critical and spice_error);
> - there are conditional logging function (spice_warn_if_reached,
>   spice_warn_if_fail, spice_warn_if);
> - there are function that mix logging, check and return
>   (spice_return_if_fail, spice_return_val_if_fail);
> - spice_printerr to log on standard error
> - spice_static_assert to do compile time checks.
> 
> Problems:
> - the Spice_style.odt is outdated;

yes

> - spice_assert recall the standard assert but instead it mainly
>   call spice_error which like GLib is fatal and should call exit/abort.
>   It is not affected by NDEBUG or other definitions. Why using it instead
>   of spice_error?

Similar to g_assert(expr) vs g_error(str).

> - you can set SPICE_ABORT_LEVEL enviroment to make error levels not
>   fatal but this is against the definition of error in the library;

Note: spice-gtk uses -DSPICE_DISABLE_ABORT, because abort() is considered pretty bad for "recoverable" errors (return_if.. etc)

> - spice_static_assert is implemented as a runtime check. As we are
>   going to use glib extensively and as glib as a macro for this
>   I would suggest to use glib macro(s).

that was the original idea, to ease the transition to glib (before glib was introduced as a dep)

> 
> Style:
> - why use spice_printerr if all errors basically go to standard error?

spice_printerr() purpose is only to print something on stderr

> - from a first time readers spice_return_if_fail and spice_return_val_if_fail
>   do not suggest function is doing some logging. Also they use critical
>   level which by default cause program abortion.

they follow the behaviour of g_return*
 
> 
> Personal considerations:
> - it seems some changes in patchset change "if (cond) return" code with
>   spice_return_if_fail(cond). As stated before the macro are very misleading
>   and the result is usually a program abortion instead of a return.
>   I think new macro with better naming (I don't have a specific suggestion)
>   would be better. The code is actually less readable with these macros;
> - although is sometimes useful to have errors reported to detect problems
>   we should consider we should consider that too logging can cause DoS
>   due to disk consumption, slow down (there are some checks on hot paths
>   like Quic or Lz). On Linux kernel there are macros like WARN_ONCE to
>   detect problems but limit the logging. We could use something like this;
> - warning or error? There are lot of changes that change spice_error to
>   spice_warning without even a comment. The behavior is really different.
>   Personally if a check is checking for memory corruption (double free,
>   overflow and so on) should be fatal. spice-server is server code
>   which should be subject to security checks and considerations.

abort() is quite bad, there are many cases where the server could have simply warned and returned and saved the VM from crashing

> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>