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