[PATCH] Wayland logging functionality

[Pekka Paalanen]

On Tue, 22 May 2012 13:20:12 +0200
Martin Minarik <minarik11 at student.fiit.stuba.sk> wrote:

> The core libwayland library should not handle logging itself. Instead,
> it returns the error in the API for anybody to subscribe to. The logging
> itself happens in weston or wayland demo applications.

Hi Martin,

thanks for tackling this. Comments inline.

> The weston and other users of libwayland API can also create their own
> custom message handlers and send their own messages to them.
> ---
>  src/wayland-server.c |    2 ++
>  src/wayland-util.c   |   39 +++++++++++++++++++++++++++++++++++++++
>  src/wayland-util.h   |   16 ++++++++++++++++
>  3 files changed, 57 insertions(+), 0 deletions(-)
> 
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index c1be56a..c846464 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -880,6 +880,8 @@ wl_display_create(void)
>  	if (debug)
>  		wl_debug = 1;
>  
> +	wl_log_set_default(wl_log_printf_handler);

Let's have noop the default. But...

> +
>  	display = malloc(sizeof *display);
>  	if (display == NULL)
>  		return NULL;
> diff --git a/src/wayland-util.c b/src/wayland-util.c
> index 7bf8924..afba3d4 100644
> --- a/src/wayland-util.c
> +++ b/src/wayland-util.c
> @@ -24,6 +24,7 @@
>  #include <stdlib.h>
>  #include <stdint.h>
>  #include <string.h>
> +#include <stdarg.h>
>  
>  #include "wayland-util.h"
>  #include "wayland-private.h"
> @@ -270,3 +271,41 @@ wl_map_for_each(struct wl_map *map, wl_iterator_func_t func, void *data)
>  	for_each_helper(&map->client_entries, func, data);
>  	for_each_helper(&map->server_entries, func, data);
>  }
> +
> +/* The pointer to default log handler */
> +static wl_log_func_t wl_log_default_handler;

You should be able to initialise it to the noop handler here, so that
calling wl_display_create does not change it. That way an application
may set a different handler before calling any other entry points.

We build two different libraries from these sources, and both libraries
may be in use at the same time in the same process. Therefore I think
we need separate entries (the variable and API functions) for
libwayland-server and libwayland-client.

You can add this into, say, wayland-private.h

extern wl_log_func_t wl_log_handler;

And then in wayland-util.c:

static void
log_noop_handler(const char *fmt, va_list ap)
{
}

wl_log_func_t wl_log_handler = log_noop_handler;

That should give us a copy of wl_log_handler in each of
libwayland-server and libwayland-client. Then you just unconditionally
use wl_log_handler in the private logging function. The handler
variable is not exported, so it should not get mixed up.

The API functions to set the handler need to be in wayland-server.[ch]
and wayland-client.[ch] with different names:
wl_log_set_handler_server
wl_log_set_handler_client

That way an application that links to both libraries, can set these
independently, and be sure the setting goes to the right lib. If we
have the same named function in both libs, knowing which one is being
called is hard, and calling the other one even harder.

(krh, maybe we should have a 'make check' test that verifies that the
symbols in server and client libs do not clash?)

> +
> +WL_EXPORT void
> +wl_log_printf_handler(const char *message, void * args)
> +{
> +	va_list *argp = args;
> +	vprintf(message, *argp);
> +}

No need for this.

> +
> +WL_EXPORT void
> +wl_log_null_handler(const char *message, void * args){}
> +
> +WL_EXPORT void
> +wl_log_set_default(wl_log_func_t handler)
> +{
> +	wl_log_default_handler = handler;
> +}
> +
> +WL_EXPORT
> +void wl_log_to(wl_log_func_t * handler, const char *message, ...)
> +{
> +	va_list argp;
> +	va_start(argp, message);
> +	(*handler)(message, &argp);
> +	va_end(argp);
> +}

No need for this either.

> +
> +WL_EXPORT void
> +wl_log(const char *message, ...)
> +{
> +	va_list argp;
> +	va_start(argp, message);
> +	wl_log_default_handler(message, &argp);
> +	va_end(argp);
> +}

This must be private internal API to the library, hence not exported.
There's also a way to annotate this function for gcc so that gcc will
check the printf format against the arguments.

> +
> diff --git a/src/wayland-util.h b/src/wayland-util.h
> index fa5c6c5..1cb4bfd 100644
> --- a/src/wayland-util.h
> +++ b/src/wayland-util.h
> @@ -203,6 +203,22 @@ static inline wl_fixed_t wl_fixed_from_int(int i)
>  	return i * 256;
>  }
>  
> +typedef void (*wl_log_func_t)(const char *, void * args);

You can just say
typedef void (*wl_log_func_t)(const char *, va_list);
I think, no need to fiddle with void*.

> +
> +/* Log to default handler */
> +void wl_log(const char *message, ...);
> +#define WL_PRINTF(...) wl_log(__VA_ARGS__)
> +
> +/* Log to handler */
> +void wl_log_to(wl_log_func_t * handler, const char *message, ...);
> +#define WL_FPRINTF(...) wl_log_to(__VA_ARGS__)
> +
> +/* Implementations of log message handling */
> +void wl_log_printf_handler(const char *message, void * args);
> +void wl_log_null_handler(const char *message, void * args);
> +

The only things exposed and exported should be the handler setter
functions, one per library, from wayland-server.h and wayland-client.h.
The wl_log_func_t type is ok, too.

> +void wl_log_set_default(wl_log_func_t handler);
> +
>  #ifdef  __cplusplus
>  }
>  #endif


Thanks,
pq