[PATCH v2 0/2] Logging functions
tiago.vignatti at linux.intel.com
Wed Feb 15 06:29:28 PST 2012
On 02/14/2012 12:30 AM, Ustun Ergenoglu wrote:
> From: Üstün Ergenoğlu<ustun.ergenoglu at gmail.com>
> Here's another attempt to add some logging functionality, this time on Weston
> instead of Wayland as Thiago suggested. In addittion, I added the WL_LOG_* macros
> that include the file and line info in the printed log lines.
Thanks for sending the second round, Ustun. I still have some comments
though. Maybe I'm being picky now.. oh well :)
1. wl_log_set_log_level should be used. We have to stick it as an option
for the user start Weston and decide which level of logging he/she
wants. Check getopt_long in main().
2. you didn't addressed my previous commentary about wl_log_va: it can
be declared as static.
3. the format of the log message could be a bit smaller. So for instance
"INFO: [2012-02-15 15:52:56]: file:line" is too much. I guess date can
be removed and the time can be the first field before the INFO/etc
stamp. Also, it's more useful if we express in milliseconds (check
wl_closure_print in wayland protocol). File and line maybe only in
debugging? I dunno.
4. in the log format instead wayland.$PID.log, I'd rather have
weston.log and weston.log.old only. It's easy to ask for some just "the
last weston log", instead the guy having to figure out which PID was the
last one. Besides, there's the fact that the user will have to clean his
XDG_RUNTIME_DIR directory from time to time cause it will be filled up
with a hundred of wayland.$PID.log. So we don't need to rotate, etc.
5. WL_LOG_D, the debug doesn't make me very happy. I never found useful
these options to enable the debugging of the whole code. However I think
it's useful to tweak it per-file, or "per-logic". Say that I want to
debug the surface transformation code, then I'd go and insert a lot of
printf on _that_ part of the code only. Maybe we could do in a way that
WL_LOG_D is enabled per-file when it has a #define DEBUG? We gotta to be
more smart a bit here..
6. You used a bit different code-style from the one we're used to.
WL_LOG_* could be all lower-case; also, instead of wl_log_*, we can
maybe use w_log_info, w_log_warning, etc. I see that for static
functions you start the declaration with underline, "_". We don't do it.
Also, my impression about systemd journal? I definitely see benefits for
users hooking up with such, specially the ones that produce big
disorganized logs, because journal seems to organize well. But big
amount of data is not a typical case of a display server. Another thing
of the journal is the ability to identify unique-formatted messages, say
to feed monitoring tools or whatever. But I don't see either an use case
for that in our case. Besides, there's also the natural fact of the
logging being tied with systemd, which is one could argue (well, Weston
has already some systemd-ism). So, I just don't feel the need for a
robust logging facility on Weston.
More information about the wayland-devel