[Spice-devel] [spice-common v3] use specific subdomains for better filtering
David Jaša
djasa at redhat.com
Mon Apr 18 15:18:52 UTC 2016
On Po, 2016-04-18 at 15:58 +0200, Victor Toso wrote:
> 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.
> >
I'm OK even with behaviour change, it just needs to be accompanied with
a notice that users won't see everything with that setting.
David
> > 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
> _______________________________________________
> 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