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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel