Hey, So there has been quite some discussions about logging. To sum up my personal thoughts on this: - we should just switch to using glib for logging. This means some mass renaming will need to be done at some point. Maybe it's possible to do it on the whole refactoring branch through git filter-branch or some similar tool, if not we can keep some #define spice_warning g_warning for now. We can also deprecate SPICE_DEBUG_LEVEL/SPICE_ASSERT_LEVEL while still keeping them kind of working with glib. - spice_return_if_fail needs to be dealt with though, either we decide it's fine to have it return instead of asserting, and we can just change it to g_return_if_fail, either we decide it's not safe, and its name should more accurately reflect that it asserts rather than returning. I would not change it to g_assert() to make it easy to spot the places we need to review whether we can g_return_if_fail() or not. - when a commit from the refactoring branch changes an assert() to a spice_return_if_fail(), we need to have a proper justification as to why this is safe to do, and also use g_return_if_fail() rather than the spice version which asserts. If the patch looks safe, I would not reject it on the basis that "we don't want to look at spice_assert() changes now". When these changes are reviewed now, this means less things to audit later. Regarding use of assert() VS g_return_if_fail(). glib documentation for g_return_if_fail() says « 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. » In short, I'd use it to document programmer expectation/assumption. The most classical use is to validate the parameters passed to (usually non-static) methods. Imo it's useful to do this in more methods than just the spice-server/QEMU interface, but also for most non-static methods. Then you can also document some more assumption, like "at this point, when I call this method, its return value really should not be NULL". If we know nothing bad will happen by returning early, it's better to just do it rather than dying in an assert. Then, there are some cases where there is no real good way to deal with the unexpected situation, even though it's bad, an assert is still better than getting in an unpredictable situation (potentially crashes/...). Regarding inconsistencies detected in refcounts, I don't have a strong opinion on assert'ing VS not assert'ing, but since it can indicated memory corruption, assert'ing makes sense. We can always change it if this causes too much trouble. I'll look some more at changing spice-common/common/log.[ch] to use glib instead of its own logging, and I'll see how we can make a global search and replace on the branch which is currently being merged if there is no strong disagreement to all of this. Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel