[Spice-devel] [PATCH spice-common 0/4] RFC: add structured logging and log category
Christophe de Dinechin
cdupontd at redhat.com
Tue Jun 13 09:48:45 UTC 2017
> On 13 Jun 2017, at 09:59, Frediano Ziglio <fziglio at redhat.com> wrote:
>
> 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 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
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170613/f0728f62/attachment-0001.html>
More information about the Spice-devel
mailing list