[PATCH weston v3 6/6] Add API to install protocol loggers on the server wl_display
Giulio Camuffo
giuliocamuffo at gmail.com
Thu Aug 11 15:06:02 UTC 2016
2016-08-11 16:39 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
> 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.
Well, we also have two different wl_display, so even if we add a
client logger, we just have to make sure we don't mix them.
>
> 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.
I'm not sure what you're saying. The logger reports the fd value, then
the user can check what it is, or just report the value just like
WAYLAND_DEBUG does.
>
>> +
>> +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.
Right.
>
>
>> + 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?
Will do.
>
>> + 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.
I have fixed the issues locally, will send a v4 shortly.
>
> 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.
Sure, i will make a web patch with some info once these patches and
the patches for the tool are merged.
Thanks,
Giulio
>
>
> Thanks,
> pq
More information about the wayland-devel
mailing list