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

[Marc-André Lureau]

Hi

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

Most of the code is to deal with deprecated SPICE_{DEBUG,ABORT}_LEVEL.

And it's there because of historical reasons, when we didn't depend on glib.

Now that we switched to glib, and assuming we had a transition period, and that these changes are not too disruptive, I think it's the right move.

> 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 will use the "old" glog handler (or the log handler of the app).

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

It will log on stderr (which may be redirected on those system).

There is a fixme in glib code to log structured log to Windows Event Log

Someone could try, perhaps based on my old attempt, to add Event Log support in glib:
https://bugzilla.gnome.org/show_bug.cgi?id=692979

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

True, I suppose it could be mitigated with helper functions.

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

Wishful thinking: there is no single log API to rule them all. A lot of domain/category/unit will want to wrap the g_log/splice_log API in some way or another.

The point it that the bare API is g_log, then if you want to add category, you can use spice_log. On top of that, there will be CHANNEL_DEBUG() etc..

I agree spice_log() could be renamed spice_debug(), and we could have different levels.

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

Yes, but I would land categories first, as we already have various G_LOG domains (or subdomains).

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