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. Talking about common things it seems we quite agree to not using multiple domains calling GLib but just use "Spice". Maybe we can code 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 list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel