[PATCH v2 0/2] Logging functions

[Tiago Vignatti]

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.

                             Tiago