[PATCH weston v3 6/6] Add API to install protocol loggers on the server wl_display
Pekka Paalanen
ppaalanen at gmail.com
Thu Aug 11 14:39:30 UTC 2016
On Tue, 9 Aug 2016 12:46:56 +0200
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> The new wl_display_add_protocol_logger allows to set a function as
> a logger, which will get called when a new request is received or an
> event is sent.
> This is akin to setting WAYLAND_DEBUG=1, but more powerful because it
> can be enabled at run time and allows to show the log e.g. in a UI view.
> A test is added for the new functionality.
>
> Signed-off-by: Giulio Camuffo <giulio.camuffo at kdab.com>
> ---
>
> v3: - added missing test file
> - renamed wl_protocol_logger_direction to wl_protocol_logger_type
> - renamed closure_log to log_closure
> - renamed wl_display_remove_protocol_logger to wl_protocol_logger_destroy
> - improved documentation
>
> Makefile.am | 5 +-
> src/wayland-server-core.h | 24 +++++++
> src/wayland-server.c | 103 ++++++++++++++++++++++++++++--
> tests/protocol-logger-test.c | 146 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 271 insertions(+), 7 deletions(-)
> create mode 100644 tests/protocol-logger-test.c
>
> diff --git a/Makefile.am b/Makefile.am
> index e684a87..3eb6fd5 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -161,7 +161,8 @@ TESTS = \
> resources-test \
> message-test \
> headers-test \
> - compositor-introspection-test
> + compositor-introspection-test \
> + protocol-logger-test
>
> if ENABLE_CPP_TEST
> TESTS += cpp-compile-test
> @@ -220,6 +221,8 @@ message_test_SOURCES = tests/message-test.c
> message_test_LDADD = libtest-runner.la
> compositor_introspection_test_SOURCES = tests/compositor-introspection-test.c
> compositor_introspection_test_LDADD = libtest-runner.la
> +protocol_logger_test_SOURCES = tests/protocol-logger-test.c
> +protocol_logger_test_LDADD = libtest-runner.la
> headers_test_SOURCES = tests/headers-test.c \
> tests/headers-protocol-test.c \
> tests/headers-protocol-core-test.c
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index 56e8d80..8e118dc 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -522,6 +522,30 @@ wl_shm_buffer_create(struct wl_client *client,
> void
> wl_log_set_handler_server(wl_log_func_t handler);
>
> +enum wl_protocol_logger_type {
> + WL_PROTOCOL_LOGGER_REQUEST,
> + WL_PROTOCOL_LOGGER_EVENT,
> +};
> +
> +struct wl_protocol_logger_message {
> + struct wl_resource *resource;
> + int message_opcode;
> + const struct wl_message *message;
> + int arguments_count;
> + const union wl_argument *arguments;
> +};
> +
> +typedef void (*wl_protocol_logger_func_t)(void *user_data,
> + enum wl_protocol_logger_type direction,
> + const struct wl_protocol_logger_message *message);
> +struct wl_protocol_logger;
> +struct wl_protocol_logger *
> +wl_display_add_protocol_logger(struct wl_display *display,
> + wl_protocol_logger_func_t, void *user_data);
Hi Giulio,
the API looks ok to me. Just a thought, might there some day be need
for a client-side protocol logger like this one? I'm thinking about
possible symbol name clashes. I suppose we can just add "_client"
somewhere in the names that need a different implementation.
Btw. have you tested this can properly report also file descriptors?
I think the file descriptor the logger sees is always a dup'd one, so
just looking at the value wouldn't tell much, you need to ask the
kernel what it points to.
> +
> +void
> +wl_protocol_logger_destroy(struct wl_protocol_logger *logger);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index fb44f13..2113cd3 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -95,6 +95,7 @@ struct wl_display {
> struct wl_list global_list;
> struct wl_list socket_list;
> struct wl_list client_list;
> + struct wl_list protocol_loggers;
>
> struct wl_signal destroy_signal;
> struct wl_signal create_client_signal;
> @@ -123,8 +124,42 @@ struct wl_resource {
> wl_dispatcher_func_t dispatcher;
> };
>
> +struct wl_protocol_logger {
> + struct wl_list link;
> + wl_protocol_logger_func_t func;
> + void *user_data;
> +};
> +
> static int debug_server = 0;
>
> +static void
> +log_closure(struct wl_resource *resource,
> + struct wl_closure *closure, int send)
> +{
> + struct wl_object *object = &resource->object;
> + struct wl_display *display = resource->client->display;
> + struct wl_protocol_logger *protocol_logger;
> + struct wl_protocol_logger_message message;
> +
> + if (debug_server)
> + wl_closure_print(closure, object, send);
> +
> + if (!wl_list_empty(&display->protocol_loggers)) {
> + message.resource = resource;
> + message.message_opcode = closure->opcode;
> + message.message = closure->message;
> + message.arguments_count = closure->count;
> + message.arguments = closure->args;
> + wl_list_for_each(protocol_logger,
> + &display->protocol_loggers, link) {
> + protocol_logger->func(protocol_logger->user_data,
> + send ? WL_PROTOCOL_LOGGER_EVENT :
> + WL_PROTOCOL_LOGGER_REQUEST,
> + &message);
> + }
> + }
> +}
> +
> WL_EXPORT void
> wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
> union wl_argument *args)
> @@ -143,8 +178,7 @@ wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
> if (wl_closure_send(closure, resource->client->connection))
> resource->client->error = 1;
>
> - if (debug_server)
> - wl_closure_print(closure, object, true);
> + log_closure(resource, closure, true);
>
> wl_closure_destroy(closure);
> }
> @@ -183,8 +217,7 @@ wl_resource_queue_event_array(struct wl_resource *resource, uint32_t opcode,
> if (wl_closure_queue(closure, resource->client->connection))
> resource->client->error = 1;
>
> - if (debug_server)
> - wl_closure_print(closure, object, true);
> + log_closure(resource, closure, true);
>
> wl_closure_destroy(closure);
> }
> @@ -331,8 +364,7 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
> break;
> }
>
> - if (debug_server)
> - wl_closure_print(closure, object, false);
> + log_closure(resource, closure, false);
>
> if ((resource_flags & WL_MAP_ENTRY_LEGACY) ||
> resource->dispatcher == NULL) {
> @@ -884,6 +916,7 @@ wl_display_create(void)
> wl_list_init(&display->socket_list);
> wl_list_init(&display->client_list);
> wl_list_init(&display->registry_resource_list);
> + wl_list_init(&display->protocol_loggers);
>
> wl_signal_init(&display->destroy_signal);
> wl_signal_init(&display->create_client_signal);
> @@ -960,6 +993,8 @@ wl_display_destroy(struct wl_display *display)
>
> wl_array_release(&display->additional_shm_formats);
>
> + wl_list_remove(&display->protocol_loggers);
> +
> free(display);
> }
>
> @@ -1480,6 +1515,62 @@ wl_log_set_handler_server(wl_log_func_t handler)
> wl_log_handler = handler;
> }
>
> +/** Adds a new protocol logger.
> + *
> + * When a new protocol message arrives or is sent from the server
> + * all the protocol logger functions will be called, carrying the
> + * \a user_data pointer, the direction of the message (incoming or
> + * outgoing) and the actual message.
> + * The lifetime of the messages passed to the logger function ends
> + * when they return so the messages cannot be stored and accessed
> + * later.
> + *
> + * \a errno is set on error.
> + *
> + * \param func The function to call to log a new protocol message
> + * \param user_data The user data pointer to pass to \a func
> + *
> + * \return The protol logger object on success, NULL on failure.
> + *
> + * \sa wl_protocol_logger_destroy
> + *
> + * \memberof wl_display
> + */
> +WL_EXPORT struct wl_protocol_logger *
> +wl_display_add_protocol_logger(struct wl_display *display,
> + wl_protocol_logger_func_t func, void *user_data)
> +{
> + struct wl_protocol_logger *logger;
> +
> + logger = malloc(sizeof *logger);
> + if (!logger)
> + return NULL;
> +
> + logger->func = func;
> + logger->user_data = user_data;
> + wl_list_init(&logger->link);
No need to init, insert overwrites the link anyway.
> + wl_list_insert(&display->protocol_loggers, &logger->link);
> +
> + return logger;
> +}
> +
> +/** Destroys a protocol logger.
> + *
> + * This function destroys a protocol logger and removes it from the display
> + * it was added to with \a wl_display_add_protocol_logger.
> + * The \a logger object becomes invalid after calling this function.
> + *
> + * \sa wl_display_add_protocol_logger
> + *
> + * \memberof wl_protocol_logger
> + */
> +WL_EXPORT void
> +wl_protocol_logger_destroy(struct wl_protocol_logger *logger)
> +{
> + wl_list_remove(&logger->link);
> + free(logger);
> +}
Ahh, good thing this stuff is server-side only, no need to think about
threads. :-)
> +
> /** Add support for a wl_shm pixel format
> *
> * \param display The display object
> diff --git a/tests/protocol-logger-test.c b/tests/protocol-logger-test.c
> new file mode 100644
> index 0000000..b86c047
> --- /dev/null
> +++ b/tests/protocol-logger-test.c
> @@ -0,0 +1,146 @@
> +/*
> + * Copyright © 2016 Klarälvdalens Datakonsult AB, a KDAB Group company, info at kdab.com
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining
> + * a copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sublicense, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial
> + * portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include <stdlib.h>
> +#include <assert.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <sys/un.h>
> +#include <unistd.h>
> +
> +#include "wayland-client.h"
> +#include "wayland-server.h"
> +#include "test-runner.h"
> +
> +/* Ensure the connection doesn't fail due to lack of XDG_RUNTIME_DIR. */
> +static const char *
> +require_xdg_runtime_dir(void)
> +{
> + char *val = getenv("XDG_RUNTIME_DIR");
> + assert(val && "set $XDG_RUNTIME_DIR to run this test");
> +
> + return val;
> +}
> +
> +struct compositor {
> + struct wl_display *display;
> + struct wl_event_loop *loop;
> + int message;
> + struct wl_client *client;
> +};
> +
> +struct message {
> + enum wl_protocol_logger_type type;
> + const char *class;
> + int opcode;
> + const char *message_name;
> + int args_count;
> +} messages[] = {
> + {
> + .type = WL_PROTOCOL_LOGGER_REQUEST,
> + .class = "wl_display",
> + .opcode = 0,
> + .message_name = "sync",
> + .args_count = 1,
> + },
> + {
> + .type = WL_PROTOCOL_LOGGER_EVENT,
> + .class = "wl_callback",
> + .opcode = 0,
> + .message_name = "done",
> + .args_count = 1,
> + },
> + {
> + .type = WL_PROTOCOL_LOGGER_EVENT,
> + .class = "wl_display",
> + .opcode = 1,
> + .message_name = "delete_id",
> + .args_count = 1,
> + },
> +};
> +
> +static void
> +logger_func(void *user_data, enum wl_protocol_logger_type type,
> + const struct wl_protocol_logger_message *message)
> +{
> + struct compositor *c = user_data;
> + struct message *msg = &messages[c->message++];
> +
> + assert(msg->type == type);
> + assert(strcmp(msg->class, wl_resource_get_class(message->resource)) == 0);
> + assert(msg->opcode == message->message_opcode);
> + assert(strcmp(msg->message_name, message->message->name) == 0);
> + assert(msg->args_count == message->arguments_count);
> +
> + c->client = wl_resource_get_client(message->resource);
> +}
> +
> +static void
> +callback_done(void *data, struct wl_callback *cb, uint32_t time)
> +{
> + wl_callback_destroy(cb);
> +}
> +
> +static const struct wl_callback_listener callback_listener = {
> + callback_done,
> +};
> +
> +TEST(logger)
> +{
> + const char *socket;
> + struct compositor compositor = { 0 };
> + struct {
> + struct wl_display *display;
> + struct wl_callback *cb;
> + } client;
> + struct wl_protocol_logger *logger;
> +
> + require_xdg_runtime_dir();
> +
> + compositor.display = wl_display_create();
> + compositor.loop = wl_display_get_event_loop(compositor.display);
> + socket = wl_display_add_socket_auto(compositor.display);
> +
> + logger = wl_display_add_protocol_logger(compositor.display,
> + logger_func, &compositor);
> +
> + client.display = wl_display_connect(socket);
> + client.cb = wl_display_sync(client.display);
> + wl_callback_add_listener(client.cb, &callback_listener, NULL);
> + wl_display_flush(client.display);
> +
> + while (compositor.message < 3) {
> + wl_event_loop_dispatch(compositor.loop, -1);
This test could use a failsafe timeout. Use test_set_timeout() on start?
> + wl_display_flush_clients(compositor.display);
> + }
> +
> + wl_display_dispatch(client.display);
> + wl_display_disconnect(client.display);
> +
> + wl_client_destroy(compositor.client);
> + wl_protocol_logger_destroy(logger);
> + wl_display_destroy(compositor.display);
> +}
Patches 4-6 are almost perfect and I could land them if you really
don't want to polish anymore.
The tool that you use these features with, it would be really nice to
have a link to it in the Wayland web page along the other debugging
tools already there.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160811/e5f3b325/attachment-0001.sig>
More information about the wayland-devel
mailing list