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

[Frediano Ziglio]

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 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.
> 
> 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