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

[Frediano Ziglio]

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

Style:
- why use spice_printerr if all errors basically go to standard error?
- 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.
  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.

Frediano