I have strange feelings about this series.
First you removed most of logging test and change the entirely logging.
This is for me an enough reason for a nack. Usually the test define
the behaviour of the API (in this case logging) and the fact you have
to change a lot means that the new one is not compatible. Considering
that this API is used by other projects and is not compatible seems
to indicate that other projects are now broken. There are other
possibilities like the test was too strict but there are no much
comments on this.
About compatibility as Christophe F said you entirely wiped out
the current environment settings which is one of the reason of
code mess.
If we are moving to structures logging I would like to move to this
always, not having different output based on different setting which
means:
- if GLib do not support structured logging we MUST write a replacement
implementing structured logging (this is currently needed for RHEL6);
- it seems this structured logging is really systemd oriented, what
about Mac OS X and Windows? One of the point of Christophe D
proposal is portability.
- the current GLib API for structure logging cannot use GNU printf
attribute making the printf potentially unsafe, I don't like this,
having the program potentially crashing on code that should be
just reading data or printing weird data is not that great. But
probably this can be fixed in some way.
In patch 1 you switch to GLib entirely, in patch 2 you remove spice_*
logging calls but then in patch 3 to support categorization you add
again spice_log. So at the end the new API is still inconsistent.
I would prefer then to keep using spice_*. If spice_log was a small
wrapper to fix some GLib logging issue I would accept a spice_log
but the spice_log is adding feature so is far from a small wrapper.
Also the spice_log is confusing and limited. Confusing as is not
clear the logging level (maybe should be spice_debug) and limited
as it only support a level of logging (the debug one).
Your patch share with Christophe D the manual code for categorization
which is implemented as an extension to GLib. So the 2 implementation
share the need to add categorization using manual code.
In structured logging mode, there is some automated categorization, like “per file”.
I believe that good categorization has to be manual.
One significant difference between the two approaches is whether the
category declaration is local (Marc-André) or whether categories are declared in a
single location (Christophe D). It’s a complicated-enough trade-off that there
is no clear winner between the two.
Local declaration pros:
- Approach works if implemented in glib rather than spice
- Categories declarations can be put close to code using them
- No big recompile when adding / editing a single category
- Only relevant categories in each component (e.g. server does not have client categories)
- More amenable to loops (as opposed to replicated code)
Global declarations pros:
- Obeys the DRY principle, as opposed to WET category declarations
(e.g. SPICE_LOG_CATEGORY_DECLARE vs. SPICE_LOG_CATEGORY)
- Makes it possible to organize categories logically, e.g. for help output
- Easier to find what categories already exists (no need to grep all source)
- Category changes impact a single file, easier for history or merge
- Categories shared between client and server, easier e.g. for remote activation
- Category errors (duplicate, missing) detected early by compiler, rather than linker
- Does not require glib at all, so would work e.g. in Windows driver
- Whole state represented as a single C struct, easy to save/restore, print in debugger, etc
- Better memory locality (not wasting a whole cache line for a single flag)
Talking about common things it seems we quite agree to not usingmultiple domains calling GLib but just use "Spice". Maybe we cancode this as a preparation change before these changes.
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.
It is useful to be able to filter the logging by domains, but GLib
only provides domain selection with G_MESSAGES_DEBUG, and it's
recommended to have few domains per application/library. Also, domain
filtering code in glib isn't very efficient, and can't be listed
easily.
A common solution to this problem is to add some form of "sub-domain"
or "categories" on top of glib (see for ex gtkdebug.h). I propose a
simple way to register such log categories, to list and selectively
enable them with the SPICE_DEBUG= environment variable, as well as
related convenience macros.
Comments welcome,
Marc-André Lureau (4):
log: replace spice log with glib
Replace spice_* logging with g_*
RFC: Add spice log categories
Add 'common_ssl' Spice log
common/canvas_base.c | 148 ++++++++++-----------
common/canvas_utils.c | 18 +--
common/gdi_canvas.c | 82 ++++++------
common/log.c | 208 +++++++----------------------
common/log.h | 175 ++++++++++++++----------
common/lz.c | 22 +--
common/lz_decompress_tmpl.c | 18 +--
common/marshaller.c | 2 +-
common/mem.c | 2 +-
common/pixman_utils.c | 156 +++++++++++-----------
common/quic.c | 88 ++++++------
common/quic_family_tmpl.c | 6 +-
common/quic_rgb_tmpl.c | 32 ++---
common/quic_tmpl.c | 32 ++---
common/rect.h | 2 +-
common/region.c | 2 +-
common/ring.h | 26 ++--
common/rop3.c | 10 +-
common/snd_codec.c | 18 +--
common/ssl_verify.c | 74 ++++++-----
common/sw_canvas.c | 20 +--
tests/test-logging.c | 317
+-------------------------------------------
22 files changed, 544 insertions(+), 914 deletions(-)
Frediano_______________________________________________Spice-devel mailing listSpice-devel@xxxxxxxxxxxxxxxxxxxxxhttps://lists.freedesktop.org/mailman/listinfo/spice-devel