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

[Victor Toso]

Hi,

On Tue, Nov 03, 2015 at 08:15:14AM -0500, Frediano Ziglio wrote:
> 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;

It is on common but spice-gtk does not seem to use them.
spice-gtk/src/spice-util.h define SPICE_DEBUG (using g_debug) for
instance. My grep returns 5 calls to spice_warning and that's it.

Is there any good reason to have macros for this? spice-server could
rely entirely on glib functions for that and setting the right log
domains which *really* helps parsing/filtering content at runtime.


> - 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;
> - 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?
> - you can set SPICE_ABORT_LEVEL enviroment to make error levels not
>   fatal but this is against the definition of error in the library;

This is probably for debuggin purpose, no?

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

Agreed!

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

Better would be that errors go to 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.
>
> 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.

But should it really abort? I understand that this is server code but
due some non-critical bug making the entirely server crash seems bad.

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

Agreed.

>
> Frediano

  Victor Toso