[PATCH v3 1/3] Weston: log.c, log.h

[Pekka Paalanen]

Hi Martin,

comments inline.


On Tue, 29 May 2012 21:44:53 +0200
Martin Minarik <minarik11 at student.fiit.stuba.sk> wrote:

> Changes from last time, we have function
> weston_log_file_create_in_home()  for opening
> log file in home directory, it can handle long file names.
> 
> We have function weston_log_handle_server() to redirect messages from
> libwayland-server into log.c:weston_logfile_handler, from
> there everything can be redirected as necessary.
> ---
>  src/Makefile.am |    2 +
>  src/log.c       |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/log.h       |   33 +++++++++++++++++++
>  3 files changed, 132 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 52457ac..27bac08 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/log.c b/src/log.c
> new file mode 100644
> index 0000000..83267d8
> --- /dev/null
> +++ b/src/log.c
> @@ -0,0 +1,97 @@
> +/*
> + * 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 <wayland-server.h>
> +#include <wayland-util.h>
> +
> +#include "log.h"
> +
> +static FILE *weston_logfile = NULL;
> +
> +static int
> +weston_log_noop_handler(const char *fmt, va_list arg)
> +{
> +	return 0;
> +}

Noop handler is not needed in weston, the default should be printing to
stderr, unless a log file is specified with a command line option.

> +
> +static int
> +weston_log_fprintf_logfile_handler(const char *fmt, va_list arg)
> +{
> +	int l;
> +	l = vfprintf(weston_logfile, fmt, arg);
> +	fflush(weston_logfile);
> +	return l;
> +}
> +
> +static wl_log_func_t weston_logfile_handler = weston_log_noop_handler;
> +
> +void
> +weston_log_handle_server()
> +{
> +	wl_log_set_handler_server(weston_logfile_handler);
> +}
> +
> +void
> +weston_log_file_create_in_home(const char *filename)
> +{
> +	char * home = getenv("HOME");

If HOME is not in the environment, you get NULL, not a pointer to a "".

> +	if (home[0]) {
> +		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 really prefer to just get the full file name through a command line
option to weston.

> +
> +void
> +weston_log_file_create(const char *filename)
> +{
> +	weston_logfile = fopen(filename, "w");

Append mode, please, until we have file rotation.

> +	if (weston_logfile == NULL)
> +		weston_logfile = stderr;
> +	weston_logfile_handler = weston_log_fprintf_logfile_handler;
> +}
> +
> +void
> +weston_log_file_destroy()
> +{
> +	weston_logfile_handler = weston_log_noop_handler;
> +	fclose(weston_logfile);
> +}
> +
> +WL_EXPORT int
> +weston_log(const char *fmt, ...)
> +{
> +	int l;
> +	va_list argp;
> +	va_start(argp, fmt);
> +	l = weston_logfile_handler(fmt, argp);
> +	va_end(argp);
> +	return l;
> +}
> diff --git a/src/log.h b/src/log.h
> new file mode 100644
> index 0000000..1b056e8
> --- /dev/null
> +++ b/src/log.h
> @@ -0,0 +1,33 @@
> +/*
> + * 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_handle_server(void);
> +void weston_log_file_create_in_home(const char *filename);
> +void weston_log_file_create(const char *filename);
> +void weston_log_file_destroy(void);
> +
> +int weston_log(const char *fmt, ...);
> +
> +#endif

You should squash the patch 2/3 into this one, so this code gets used
the same time it is added.

Patch 3/3 looks good, and due to its size (big) and nature (lots of
trivial changes), can stay on its own.


Thanks,
pq