[Spice-devel] SPICE logging facilities

[Frediano Ziglio]

> 
> Hi,
> 
> On Wed, Jul 04, 2018 at 11:21:56AM +0200, Christophe Fergeau wrote:
> > On Tue, Jul 03, 2018 at 05:10:23PM +0200, Christophe de Dinechin wrote:
> > > There is a lot of debate, distributed across a large number of patches,
> > > regarding similar SPICE and glib facilities. For a number of things,
> > > there are two sets of parallel APIs with slightly different behaviour.
> > > This is undesirable, as it introduces confusion.
> > > 
> > > Pros: this lets us customize the behavior for SPICE
> > > 
> > > Cons: the SPICE macros are less documented, and not obvious to tell how
> > > they differ from the glib counterpart.
> > > 
> > > 
> > > As I wrote in a response to a patch comment, I personally value
> > > consistency relatively high, so if a file currently uses
> > > spice_return_if_fail, that’s what I will use in that file. While I may
> > > agree that using g_return_if_fail in general could be preferable to
> > > spice_return_if_fail, I would like
> > > 
> > > a) to avoid having repeated and unproductive comments about “why not
> > > use the glib version” for every patch that touches one of these
> > > macros.
> > > 
> > > b) to have a clearer understanding of what the benefits of the SPICE
> > > and glib variants are (I somewhat understand the difference, my
> > > question is more about whether there is real value add to the SPICE
> > > variant or not).
> > > 
> > > 
> > > I had started explaining my preferences here, but I realize it’s
> > > probably better to gather everyone’s opinion first. Please share your
> > > preference, e.g. should we switch to glib wholesale, piecemeal, not at
> > > all, on a per-macro basis, etc, and the rationale behind your
> > > preference. Meanwhile, I’ll do some additional experiments to solidify
> > > or change my own preference.
> > 
> > Initially, spice-server/spice-common had no glib dependency, so
> > spice_warning & friends made sense. Then they started to depend
> > on glib, so some of the stuff they contained became redundant
> > with what glib provides. Then spice_log was patched to go
> > through g_log, while retaining its slight behaviourial
> > differences, mostly for backward compatibility. At this point,
> > the plan was to slowly get rid of spice_* and to move to glib
> > logging. Which is why I push for using glib logging when a
> > patch tries to use the deprecated spice logging API.  If the
> > inconsistency within a file is a problem, then the file can be
> > switched to the new API first, and the new code can use the
> > glib API and everything is consistent. Keeping using spice
> > logging API for the sake of consistency is not going to make
> > the situation better.
> > 
> > Christophe
> 
> I do like the approach of sanitizing the file(s) that the
> following up patch will touch. Should be easier to test and
> review but would take several patches to sanitize the code, etc.
> 
> Now might be a good time to start with this as we just released
> spice-gtk and a release of spice is not too far either.
> 
> And, yes, I'm in favor of removing spice related functions/macros
> that there is a glib equivalent.
> 
> Cheers,
>         toso
> 

In some way I can say I agree with everybody but I think Victor
introduced the real "key" to the question: the word "equivalent".

Let me explain. This discussion started specifically with the
spice_critical/g_critical APIs. Now are they equivalent?
Well, one call a backtrace function and shows the backtrace on
standard output the other not. So, they are not 100% equivalent.
Of course we can discuss on the value of the additional/different
behaviour but are not equivalent.

Another question is however "Are we going to use g_critical as
g_critical?". It sounds a tricky question. Let say that a new person
starts to look at the code and knows GLib. He see g_critical and
think "well, this by default log a critical warning and continue"
but instead on Spice is always fatal. This confused, and I think
is not to blame, c3d. teuf state clearly that spice_critical calls
abort. So clearly changing spice_critical to g_critical confuse
and does not help new comers. Maybe, as expected to call abort,
spice_critical should be replaced by g_error using Glib in its
default expected behaviour (by extension g_return_if_fails should
be replaced by a g_assert or similar) ?

Frediano