Hi Martin, comments inline as usual.
On Thu, 31 May 2012 21:48:23 +0200 Martin Minarik <[email protected]> wrote: > This is logging functionality for weston compositor. > > It handles: > messages coming from libwayland-server from wl_log() > messages from weston itself, from weston_log() > > Introduce --log option, to specify log file path on the command line. > When the path is incorrect, or on weston_log_file_destroy(), fall > back to stderr. > > In case no log file is provided, fall back to $HOME/weston.log. > --- > src/Makefile.am | 2 + > src/compositor.c | 8 +++ > src/log.c | 127 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > src/log.h | 34 ++++++++++++++ > 4 files changed, 171 insertions(+), 0 deletions(-) > create mode 100644 src/log.c > create mode 100644 src/log.h > > diff --git a/src/Makefile.am b/src/Makefile.am > index 7606211..4e6e609 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -11,6 +11,8 @@ weston_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) > weston_LDADD = $(COMPOSITOR_LIBS) $(DLOPEN_LIBS) -lm ../shared/libshared.la > > weston_SOURCES = \ > + log.c \ > + log.h \ > compositor.c \ > compositor.h \ > filter.c \ > diff --git a/src/compositor.c b/src/compositor.c > index a36ccd5..fa17363 100644 > --- a/src/compositor.c > +++ b/src/compositor.c > @@ -48,6 +48,7 @@ > > #include <wayland-server.h> > #include "compositor.h" > +#include "log.h" > > static struct wl_list child_process_list; > static jmp_buf segv_jmp_buf; > @@ -2861,6 +2862,7 @@ int main(int argc, char *argv[]) > char *backend = NULL; > char *shell = NULL; > char *module = NULL; > + char *log = NULL; > int32_t idle_time = 300; > int32_t xserver = 0; > char *socket_name = NULL; > @@ -2892,6 +2894,7 @@ int main(int argc, char *argv[]) > { WESTON_OPTION_INTEGER, "idle-time", 'i', &idle_time }, > { WESTON_OPTION_BOOLEAN, "xserver", 0, &xserver }, > { WESTON_OPTION_STRING, "module", 0, &module }, > + { WESTON_OPTION_STRING, "log", 0, &log }, > }; > > memset(&xkb_names, 0, sizeof(xkb_names)); > @@ -2899,6 +2902,9 @@ int main(int argc, char *argv[]) > argc = parse_options(core_options, > ARRAY_LENGTH(core_options), argc, argv); > > + weston_log_claim_libwaylandserver(); > + weston_log_file_create_from_opt(log); > + > display = wl_display_create(); > > loop = wl_display_get_event_loop(display); > @@ -3003,5 +3009,7 @@ int main(int argc, char *argv[]) > ec->destroy(ec); > wl_display_destroy(display); > > + weston_log_file_destroy(); > + > return 0; > } > diff --git a/src/log.c b/src/log.c > new file mode 100644 > index 0000000..3665c8f > --- /dev/null > +++ b/src/log.c > @@ -0,0 +1,127 @@ > +/* > + * Copyright © 2012 Martin Minarik > + * > + * Permission to use, copy, modify, distribute, and sell this software and > + * its documentation for any purpose is hereby granted without fee, provided > + * that the above copyright notice appear in all copies and that both that > + * copyright notice and this permission notice appear in supporting > + * documentation, and that the name of the copyright holders not be used in > + * advertising or publicity pertaining to distribution of the software > + * without specific, written prior permission. The copyright holders make > + * no representations about the suitability of this software for any > + * purpose. It is provided "as is" without express or implied warranty. > + * > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS > + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND > + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY > + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER > + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF > + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN > + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > + */ > + > +#include <stdio.h> > +#include <stdarg.h> > +#include <stdlib.h> > +#include <string.h> > +#include <time.h> > + > +#include <wayland-server.h> > +#include <wayland-util.h> > + > +#include "log.h" > + > +static FILE *weston_logfile = NULL; > + > +static int weston_log_timestamp(void) > +{ > + struct timespec tp; > + unsigned int time; > + > + clock_gettime(CLOCK_REALTIME, &tp); > + time = (tp.tv_sec * 1000000L) + (tp.tv_nsec / 1000); > + > + return fprintf(weston_logfile, "[%10.3f] ", time / 1000.0); Could we get the timestamp in seconds after weston's start? Hmm, or should we put there the date like in 2012-06-01 13:08:47.023 bla bla blah perhaps? For me it's hard to say, was [2359334.390] (as found in Xorg.log, for instance) a moment ago or last week. > +} > + > +static int weston_log_print(const char *fmt, va_list arg) > +{ > + int l; > + l = vfprintf(weston_logfile, fmt, arg); > + fflush(weston_logfile); > + return l; > +} > + > +static void > +custom_handler(const char *fmt, va_list arg) > +{ > + weston_log_print(fmt, arg); This one should add the timestamp, too. And "libwayland: " prefix, so it is obvious in the log, that these messages come from libwayland. > +} > + > +void > +weston_log_claim_libwaylandserver() > +{ > + wl_log_set_handler_server(custom_handler); > +} > + > +void > +weston_log_file_create_from_opt(char *filename) > +{ > + if (filename == NULL) { > + filename = ""; > + } > + > + if (filename[0] == 0) { Remove these 4 lines above, and it's fine. weston_log_file_create() will return an error if creating the file fails, anyway. > + filename = "/weston.log"; > + char * home = getenv("HOME"); > + if (home != NULL) { > + char * string; > + int length = strlen(home) + strlen(filename) + 1; > + string = malloc((length+1) * sizeof(string)); > + snprintf(string, length, "%s%s", home, filename); > + weston_log_file_create(string); > + free(string); I'd rather fall back to stderr than a hardcoded file straight under my $HOME. > + } > + } else { > + weston_log_file_create(filename); > + } > +} I think weston_log_claim_libwaylandserver() and weston_log_file_create_from_opt(char *filename) should be combined to just weston_log_open(const char *filename). > + > +void > +weston_log_file_create(const char *filename) > +{ > + weston_logfile = fopen(filename, "w"); Append mode, please. > + if (weston_logfile == NULL) > + weston_logfile = stderr; > +} > + > +void > +weston_log_file_destroy() Would prefer the name weston_log_close(), to pair nicely with weston_log_open(). We are not destroying the log. ;-) > +{ > + if (weston_logfile != stderr) > + fclose(weston_logfile); > + weston_logfile = stderr; > +} > + > +WL_EXPORT int > +weston_log(const char *fmt, ...) > +{ > + int l; > + va_list argp; > + va_start(argp, fmt); > + l = weston_log_timestamp(); > + l += weston_log_print(fmt, argp); > + va_end(argp); > + return l; > +} > + > +WL_EXPORT int > +weston_log_continue(const char *fmt, ...) > +{ > + int l; > + va_list argp; > + va_start(argp, fmt); > + l = weston_log_print(fmt, argp); > + va_end(argp); > + return l; > +} Looks good. > diff --git a/src/log.h b/src/log.h > new file mode 100644 > index 0000000..08a93d0 > --- /dev/null > +++ b/src/log.h > @@ -0,0 +1,34 @@ > +/* > + * Copyright © 2012 Martin Minarik > + * > + * Permission to use, copy, modify, distribute, and sell this software and > + * its documentation for any purpose is hereby granted without fee, provided > + * that the above copyright notice appear in all copies and that both that > + * copyright notice and this permission notice appear in supporting > + * documentation, and that the name of the copyright holders not be used in > + * advertising or publicity pertaining to distribution of the software > + * without specific, written prior permission. The copyright holders make > + * no representations about the suitability of this software for any > + * purpose. It is provided "as is" without express or implied warranty. > + * > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS > + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND > + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY > + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER > + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF > + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN > + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > + */ > + > +#ifndef _WAYLAND_SYSTEM_LOG_H_ > +#define _WAYLAND_SYSTEM_LOG_H_ > + > +void weston_log_claim_libwaylandserver(void); > +void weston_log_file_create_from_opt(char *filename); > +void weston_log_file_create(const char *filename); > +void weston_log_file_destroy(void); weston_log_file_create() doesn't need to be here, it is private to log.c. In my opinion, the API should be like: > + void weston_log_open(const char *fname); void weston_log_close(void); > +int weston_log(const char *fmt, ...); > +int weston_log_continue(const char *fmt, ...); > + > +#endif Thank you for your patience :-) - pq _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
