[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 07:22:07 UTC 2016
Hi,
2016-08-10 18:28 GMT+02:00 Yong Bakos <junk at humanoriented.com>:
> Hi Giulio,
>
>> On Aug 9, 2016, at 3:46 AM, 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>
>
> If this series goes to v4 I have some minor suggestions inline below,
> including the test toward the end.
> Otherwise, this is:
> Reviewed-by: Yong Bakos <ybakos at humanoriented.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;
>
> Needs line breaks, or better yet just remove the forward declaration.
> It's not needed since you're getting that for free with the return
> type in the next line.
>
>
>> +struct wl_protocol_logger *
>> +wl_display_add_protocol_logger(struct wl_display *display,
>> + wl_protocol_logger_func_t, void *user_data);
>> +
>> +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;
>>
>
>
> Would be nice to see a comment header here, but we can add
> after the merge.
Sorry, a comment about what?
>
>> +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);
>
> Are you sure it makes sense to couple these?
Well, they are only used together so i don't see why not.
>
>
>> +
>> + 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) {
>
> Sanity check: `link` and not `protocol_logger->link`, correct?
Yes, 'link' is the name of the field.
>
>
>> + 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);
>> + 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);
>> +}
>> +
>> /** 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);
>
> Maybe it makes sense to add two here, and testing that both
> logger funcs get invoked.
>
> Regards,
> yong
>
>
>> +
>> + 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);
>> + 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);
>> +}
>> --
>> 2.9.2
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list