On 15 February 2012 16:29, Tiago Vignatti <tiago.vigna...@linux.intel.com> wrote: > On 02/14/2012 12:30 AM, Ustun Ergenoglu wrote: >> >> From: Üstün Ergenoğlu<ustun.ergeno...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
