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