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

[Christophe Fergeau]

Hey,

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

Yes, not very clear whether this is intentional or not. The commit which
introduced them is
http://cgit.freedesktop.org/spice/spice-common/commit/?id=c1403ee6bf4dfdd8f614f84ef145083b06a9f23e
which replaces some ASSERT() with spice_assert(), some others with
spice_return_if_fail(). The log does not mention whether these changes
are expected to return and were carefully audited, or whether they rely
on the fact that spice_return_if_fail() is going to abort().

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

Regarding this first point, I'm personally fine with the naming (or
using g_return_if_fail() instead) as long as it's clear that they have
the same meaning as in glib:

"If expr evaluates to FALSE, the current function should be considered
to have undefined behaviour (a programmer error). The only correct
solution to such an error is to change the module that is calling the
current function, so that it avoids this incorrect call.

To make this undefined behaviour visible, if expr evaluates to FALSE,
the result is usually that a critical message is logged and the current
function returns."


Ie these warnings are only meant to trigger when we hit a situation the
programmer did not expect to happen (in other words a bug). With such a
use in mind, replacing if() with spice_return_if_fail() for
unexpected/unhandled cases makes sense, especially as this makes it easy
to catch in a debugger if it's reproducible.


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

Could be useful at some point, though I don't think we had that many
issues with too verbose logging until now.


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

Yup, we should err on the safe-side here, though in the end it's going
to be resolved on a case-by-case basis anyway.

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/20151103/f579b5e3/attachment-0001.sig>