[PATCH v4 1/2] Weston: log.c, log.h

Pekka Paalanen ppaalanen at gmail.com
Fri Jun 1 03:14:14 PDT 2012


Hi Martin,

comments inline as usual.

On Thu, 31 May 2012 21:48:23 +0200
Martin Minarik <minarik11 at student.fiit.stuba.sk> 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


More information about the wayland-devel mailing list