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