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@xxxxxxxxxx> > wrote: > > > > > > On 12 Jun 2017, at 10:19, marcandre.lureau@xxxxxxxxxx wrote: > > > > > > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx> > > > > > > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel