[Spice-devel] [spice-common v3] use specific subdomains for better filtering
Victor Toso
lists at victortoso.com
Mon Apr 18 13:58:22 UTC 2016
Hi,
On Fri, Apr 15, 2016 at 04:22:05PM +0200, David Jaša wrote:
> Hi, I like the approach a lot. It seems quite clear and nice to me,
> but ...
>
> On So, 2016-03-12 at 15:32 +0100, Victor Toso wrote:
> > Hi! I've rebased this series and pushed to my remote branch in
> > freedesktop [0] [1]. I'll try to clarify the ideia for working on this
> > and if it does not get any positive feedback I'll take it as something
> > not interesting to have...
> >
> > [0] (common) https://cgit.freedesktop.org/~victortoso/spice-common/log/?h=logs
> > [1] (gtk) https://cgit.freedesktop.org/~victortoso/spice-gtk/log/?h=logs
> > fdo: https://bugs.freedesktop.org/show_bug.cgi?id=91838
> >
> > So, first of all, I don't want to introduce something that everybody
> > need to understand in order to get proper debug. The main idea is able
> > to filter in/out stuff that you don't want to see without the need to
> > | grep -v and | grep -i it.
> >
> > With that said, SPICE_DEBUG=6 should work to get G_LOG_LEVEL_DEBUG of
> > all code that uses spice logging functions.
>
> ... this essentially breaks compatibility in a bad way. IMO it has
> already "sunk in" that G_MESSAGES_DEBUG=(GSpice|all) SPICE_DEBUG=1 means
> all debug output from spice_gtk. Having just a trickle instead of
> expected flood would be surprising.
>
> I suggest adding a brief message when SPICE_DEBUG is set outlining the
> new usage so that you don't have to bother with backwards compatibility
> and users are warned.
Yes, SPICE_DEBUG should always work as boolean (agreed with Marc-Andre about it
before, I think). I still think that it is okay to filter subdomains under
SPICE_DEBUG.
>
> The same would be suitable eventually for spice server when it's
> SPICE_DEBUG_LEVEL-controlled debugging is replaced with a new one.
>
> > With all debug enable you would see all lines prefixed with
> > "[subdomain-name]" so, in case you don't want to see it (filter out),
> > you could:
> >
> > $SPICE_DEBUG=6,subdomain-name:- remote-viwer ...
> >
> > And voilà, nothing from it will be showed as '-' is the same as '0'
> > which is *no debug*. The opposite of it would be '*' or '6'.
> > * Both '-' and 0 would work and "none" as well;
> > * Both '*' and 6 would work and "debug" as well;
> >
>
> +1 to Marc-André's suggestion to remove multiple choices.
Okay :)
>
> > In case you only want to see some debug info, you could do something
> > like:
> >
> > SPICE_DEBUG=audio:*,playback:*
> >
> > This would show spice-audio, spice-gstaduio, spice-pulse,
> > channel-playback debugs and nothing else.
>
> I don't like asterisks because they need to be escaped in shell. For
> subdomains, "all" would be IMO better and for log levels, highest
> verbosity level should be sufficient. The highest level could be printed
> in the logging overview message (and either taken out of the actual
> code, or standardized in coding style to avoid surprises).
>
> HTH,
>
> David
Sure.
I'm taking a closer look to the outcome from support under glib for structured
logging API [0]. Hopefully we can reduce the work to have this RFE in spice with
this glib support... Let's see ;)
[0] https://bugzilla.gnome.org/show_bug.cgi?id=744456
Thanks for taking a look on this series,
Victor Toso
>
> >
> > Q: Do you want this just to avoid grep?
> > A: No! We should not avoid inserting debug into the code because it
> > could get verbose, we should smart handling it with something like this
> > IMHO. We have a few #ifdef #DEBUG_THIS which is not useful when asking
> > debug info to reporter in open bugs.
> >
> > I really think that this could be improved a lot yet but it would need a
> > proper discussion. If this does not seem useful, then it's fine :)
> >
> > Another misc info:
> > * I did not try this with spice server yet;
> > * We can use spice logging tools or glib ones, I choosed spice first as
> > I think we can have better control with it;
> > * I did not try to fix the spice-common tests yet;
> >
> > changes from v2:
> > * rebased to latest version upstream
> > * Kept usage of SPICE_LOG_DOMAIN instead of G_LOG_DOMAIN that I
> > previously proposed
> > * moved some changes that should be in another commit
> > * changed the static variable from SPICE_LOG_DOMAIN to
> > SPICE_LOG_SUB_DOMAIN
> >
> > changes from v1:
> > * Use one-static-per-file approach to define each subdomain. This is
> > similar to how libvirt does and seems much cleaner.
> > * Removed f(printf) debugs
> > * Created subdomains for spice-common as well as now this is a must.
> >
> > [spice-common]
> > Victor Toso (6):
> > log: simplify log defines with SPICE_LOG
> > log: include message log level for parity with glib
> > log: allow filtering logs with subdomains
> > log: create specific subdomains for filtering
> > log: Disable multiple domains in Spice
> > don't break the build with this wip patches
> >
> > common/canvas_base.c | 3 +-
> > common/canvas_utils.c | 1 +
> > common/log.c | 174 ++++++++++++++++++++++++++++++++++++++++++++++----
> > common/log.h | 87 ++++++++++++++++---------
> > common/lz.c | 2 +
> > common/marshaller.c | 1 +
> > common/mem.c | 1 +
> > common/pixman_utils.c | 1 +
> > common/quic.c | 4 +-
> > common/region.c | 2 +
> > common/rop3.c | 4 +-
> > common/ssl_verify.c | 3 +-
> > tests/test-logging.c | 3 +-
> > 13 files changed, 238 insertions(+), 48 deletions(-)
> >
> > [spice-gtk]
> > Victor Toso (13):
> > log: use glib logging on testing tools
> > log: use spice_debug instead of SPICE_DEBUG
> > log: nitpick at channel name in CHANNEL_DEBUG
> > log: remove unused SPICE_DEBUG
> > log: use spice_debug instead of g_debug
> > log: use spice_warning instead of g_warning
> > log: use spice_critical instead of g_critical
> > log: use plain spice_debug instead of VNC_DEBUG
> > log: use spice logging instead of (f)printf
> > log: use spice_error instead of g_error
> > Update spice-common submodule
> > log: create subdomains for spice-gtk
> > log: remove dup __FUNCTION__ argument from logs
> >
> > spice-common | 2 +-
> > src/Makefile.am | 3 +
> > src/bio-gio.c | 8 +-
> > src/channel-base.c | 14 +--
> > src/channel-cursor.c | 30 +++---
> > src/channel-display-mjpeg.c | 4 +-
> > src/channel-display.c | 58 +++++-----
> > src/channel-inputs.c | 4 +-
> > src/channel-main.c | 83 +++++++-------
> > src/channel-playback.c | 20 ++--
> > src/channel-port.c | 2 +
> > src/channel-record.c | 10 +-
> > src/channel-smartcard.c | 6 +-
> > src/channel-usbredir.c | 8 +-
> > src/channel-webdav.c | 16 +--
> > src/coroutine_gthread.c | 8 +-
> > src/coroutine_ucontext.c | 7 +-
> > src/coroutine_winfibers.c | 9 +-
> > src/decode-glz.c | 10 +-
> > src/decode-jpeg.c | 6 +-
> > src/decode-zlib.c | 6 +-
> > src/desktop-integration.c | 10 +-
> > src/giopipe.c | 11 +-
> > src/map-file | 1 +
> > src/smartcard-manager.c | 20 ++--
> > src/spice-audio.c | 4 +-
> > src/spice-channel-priv.h | 2 +-
> > src/spice-channel.c | 191 ++++++++++++++++++---------------
> > src/spice-client-glib-usb-acl-helper.c | 9 +-
> > src/spice-grabsequence.c | 4 +-
> > src/spice-gstaudio.c | 48 +++++----
> > src/spice-gtk-session.c | 48 +++++----
> > src/spice-option.c | 4 +-
> > src/spice-pulse.c | 142 ++++++++++++------------
> > src/spice-session.c | 60 ++++++-----
> > src/spice-uri.c | 2 +
> > src/spice-util.c | 8 +-
> > src/spice-util.h | 6 --
> > src/spice-widget-egl.c | 18 ++--
> > src/spice-widget.c | 96 +++++++++--------
> > src/spicy-connect.c | 2 +-
> > src/spicy-screenshot.c | 10 +-
> > src/spicy-stats.c | 12 +--
> > src/spicy.c | 34 +++---
> > src/usb-device-manager.c | 64 +++++------
> > src/usb-device-widget.c | 7 +-
> > src/usbutil.c | 4 +-
> > src/vmcstream.c | 14 +--
> > src/vncdisplaykeymap.c | 46 ++++----
> > src/win-usb-dev.c | 18 ++--
> > src/win-usb-driver-install.c | 46 ++++----
> > 51 files changed, 675 insertions(+), 580 deletions(-)
> >
>
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list