[PATCH v2 0/2] Logging functions

[Üstün Ergenoglu]

On 15 February 2012 16:29, Tiago Vignatti
<tiago.vignatti at linux.intel.com> wrote:
> 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().
Yeah, with this patch I had just left it in the default
level(WL_LOG_DEBUG). But I think setting the default level to
WL_LOG_INFO and let the user choose is a better approach.

>
> 2. you didn't addressed my previous commentary about wl_log_va: it can be
> declared as static.
I left it there because many other variable argument functions have a
va_list version because someone somewhere could need that but if you
think this is unnecessary so be it.

>
> 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.
This sounds better.

>
> 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..
This also sounds better. Showing the debug level output only when
DEBUG is defined. Including the file:line info with the DEBUG defined
could be better as well.

>
> 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.
I made them so because they were macros, but afterall they could be
lower case. I will change them.

-Üstün