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

[Christophe Fergeau]

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
-------------- 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/20151120/d52c3eb8/attachment-0001.sig>