[Spice-devel] [PATCH spice-common 0/4] RFC: add structured logging and log category

[Christophe Fergeau]

On Mon, Jun 12, 2017 at 09:32:38PM +0000, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jun 12, 2017 at 9:11 PM Christophe de Dinechin <dinechin at redhat.com>
> wrote:
> 
> >
> > > On 12 Jun 2017, at 10:19, marcandre.lureau at redhat.com wrote:
> > >
> > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > >
> > > Hi,
> > >
> > > This is a RFC series to clean-up Spice logging infrastructure to fully
> > > rely on glib g_log & g_log_structured if available, and add logging
> > > categories. It is thus a counter-proposal to Christophe D. "RFC:
> > > Lightweight tracing mechanism", with which it shares the category
> > > selection/listing feature.
> >
> > I do not see it as a counter proposal, but rather as an improvement to a
> > part I had not really dealt with, the logging itself. I also see this as
> > something that, as implemented, truly belongs to glib. I would happily
> > trade the slight extra costs for the benefits of having that be a shared
> > mechanism between glib applications.
> >
> >
> I'll propose it, but don't think glib will be interested, as they have the
> domain selection and strctured logging. You can do filtering of the log you
> send on top.
> 
> Here is what I like:
> >
> > 1. Can be implemented in glib, and in my opinion, would be a nice addition
> > there (replacing the spice prefix, obviously).
> >
> >
> +1
> 
> 2. Also has some self-documentation provisions (although, quite frankly,
> > expecting someone to set SPICE_DEBUG=help seems a bit contrived to me ;-)
> >
> > A DEBUG environment variable is often parsed with g_parse_debug_string()
> (G_DEBUG=help, see also glib/clutter/gtk etc), which uses 'help' to list
> all keys. Here I wanted to follow more closely G_MESSAGES_DEBUG, use glob
> matching, and list a category description too, although we may consider
> that last part quite unnecessary and may switch to g_parse eventually.
> 
> 3. Removes overhead associated with spice_log, replacing it with direct
> > calls to glib logging functions
> >
> > 4. Migrates to structured logging if available, so you get the benefits of
> > that too.
> >
> > 5. Uses “true” pattern matching as opposed to my ad-hoc syntax. Its
> > clearly more expensive, but then it’s only once at startup, so it’s better.
> >
> >
> > Here is what I feel is debatable:
> >
> > 1. Replacing spice_foo with g_foo in the code (i.e. your patch 2/4). It
> > makes sense to be able to do something special about, say,
> > spice_return_if_fail, if only make it easy to breakpoint (e.g. by adding a
> > call to some foo() function) to debug a spice CRITICAL message.
> >
> 
> I don't really get the problem here, you can put a breakpoint anywhere
> where there is a g_return* or you may use G_DEBUG=fatal-criticals to break
> on criticals and/or errors.

Or break on g_log if log_level is higher/smaller than a given value.

> > glib uses deprecation macros & tricks for doing min & max version
> checking, and spice has 2.28 as max version currently (see
> https://git.gnome.org/browse/glib/tree/glib/gversionmacros.h and other
> macros). To enable G_LOG_USER_STRUCTURED on Fedora for instance, we need to
> override the warning. This is also done in a couple of places in spice-gtk
> btw
> 
> 3. What is the comment /* because spice_debug(NULL) exists.. */ ? It does
> > not explain anything to me, frightens me even ;-)
> >
> 
> 
> spice_debug(x) should be g_debug(SPICE_LOG_DOMAIN ": " x, ## __VA_ARGS__),
> but you can't do that if you have callers with NULL.
> 
> This should go away quickly, along with _spice_debug as soon as we switch
> codebase to g_log usage and spice_log().

I'd definitely replace these with spice_debug(G_STRFUNC); in a
preparatory commit. This bothered me in the past too. I'll send one soon
if noone has beaten me to it yet.

> > Here is what I feel is the important stuff missing relative to my own
> > proposal:
> >
> > 1. No command-line option to select debug categories. Can be fixed with a
> > script wrapper, but command-line options are a good thing.
> >
> 
> Command line options are user visible, thus must be stable and friendly. I
> disagree to put anything more complicated that --spice-debug.

I'd make --spice-debug equivalent to SPICE_DEBUG=1, and accept
categories: --spice-debug=foo:bar:baz, --spice-debug=help, ..

Christophe

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170613/7ee41696/attachment-0001.sig>