[PATCH wayland v2] Add API to install protocol loggers on the server wl_display
Pekka Paalanen
ppaalanen at gmail.com
Fri Jun 17 12:58:16 UTC 2016
On Thu, 7 Apr 2016 14:37:44 +0300
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.
>
> Signed-off-by: Giulio Camuffo <giulio.camuffo at kdab.com>
> ---
>
> v2: changed the logger function to pass a struct wl_log_message* with all
> the relevant data instead of a string.
>
> src/wayland-server-core.h | 21 ++++++++++
> src/wayland-server.c | 100 +++++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 115 insertions(+), 6 deletions(-)
Hi,
I tried a few points in the git history around the date this was
posted, but I couldn't find a revision where this patch would apply.
This is an interesting idea indeed. A major point in understanding how
this feature can be used is the ptrace hookup you mentioned. That could
be worth adding in the commit message.
> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
> index 9980c29..07a42fc 100644
> --- a/src/wayland-server-core.h
> +++ b/src/wayland-server-core.h
> @@ -514,6 +514,27 @@ wl_shm_buffer_create(struct wl_client *client,
>
> void wl_log_set_handler_server(wl_log_func_t handler);
>
> +enum wl_protocol_logger_direction {
> + WL_PROTOCOL_LOGGER_INCOMING,
> + WL_PROTOCOL_LOGGER_OUTGOING,
How about calling these EVENT and REQUEST, to match our protocol
vocabulary?
> +};
> +
> +struct wl_log_message {
> + struct wl_resource *resource;
> + int message_opcode;
> + const struct wl_message *message;
> + int arguments_count;
> + const union wl_argument *arguments;
> +};
This is as expected. You can get the client and resource version from
the resource.
> +
> +typedef void (*wl_protocol_logger_func_t)(void *, enum wl_protocol_logger_direction,
> + const struct wl_log_message *);
Are the arguments of this callback documented anywhere? I couldn't find
them and they don't even have names to give a hint.
> +void wl_add_protocol_logger(struct wl_display *display,
> + wl_protocol_logger_func_t, void *user_data);
> +
> +void wl_remove_protocol_logger(struct wl_display *display,
> + wl_protocol_logger_func_t, void *user_data);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index e47ccec..18bbec8 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;
> @@ -121,10 +122,45 @@ struct wl_resource {
> void *data;
> int version;
> wl_dispatcher_func_t dispatcher;
> + struct wl_resource *parent;
Wait, what is this doing here?
> +};
> +
> +struct wl_protocol_logger {
> + struct wl_list link;
> + wl_protocol_logger_func_t func;
> + void *user_data;
> };
>
> static int debug_server = 0;
>
I am very glad to see the function-local static variables gone in this
version. Those would have been pain for someone running two different
wl_displays in threads. No, I do not know of such a user, thankfully.
> +static void
> +closure_log(struct wl_resource *resource,
> + struct wl_closure *closure, int send)
How about instead of a boolean, use the enum you defined?
That would read much more obvious in the callers.
Oh but wl_closure_print() does the bool, and it's used in both server
and client code. Bummer.
> +{
> + struct wl_object *object = &resource->object;
> + struct wl_display *display = resource->client->display;
> + struct wl_protocol_logger *protocol_logger;
> + struct wl_log_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_OUTGOING :
> + WL_PROTOCOL_LOGGER_INCOMING,
> + &message);
> + }
> + }
> +}
> +
> WL_EXPORT void
> wl_resource_post_event_array(struct wl_resource *resource, uint32_t opcode,
> union wl_argument *args)
> @@ -143,8 +179,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);
> + closure_log(resource, closure, true);
>
> wl_closure_destroy(closure);
> }
> @@ -183,8 +218,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);
> + closure_log(resource, closure, true);
>
> wl_closure_destroy(closure);
> }
> @@ -331,8 +365,7 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
> break;
> }
>
> - if (debug_server)
> - wl_closure_print(closure, object, false);
> + closure_log(resource, closure, false);
>
> if ((resource_flags & WL_MAP_ENTRY_LEGACY) ||
> resource->dispatcher == NULL) {
> @@ -879,6 +912,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);
> @@ -1463,6 +1497,60 @@ 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.
> + *
> + * \param func The function to call to log a new protocol message
> + * \param user_data The user data pointer to pass to \a func
> + *
> + * \sa wl_remove_protocol_logger
> + *
> + * \memberof wl_display
> + */
> +WL_EXPORT void
> +wl_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;
> +
> + logger->func = func;
> + logger->user_data = user_data;
> + wl_list_init(&logger->link);
> + wl_list_insert(&display->protocol_loggers, &logger->link);
> +}
How about having this function return an opaque struct
wl_protocol_logger*?
Then the remove function could operate on that instead of doing a
search. You would also be able to return a failure.
> +
> +/** Removes a protocol logger.
> + *
> + * If a protocol logger was previously added with the same \a func and
> + * \a user_data, it will be removed from the display's list so that it
> + * will not be invoked anymore.
> + *
> + * \sa wl_add_protocol_logger
> + *
> + * \memberof wl_display
> + */
> +WL_EXPORT void
> +wl_remove_protocol_logger(struct wl_display *display,
> + wl_protocol_logger_func_t func, void *user_data)
> +{
> + struct wl_protocol_logger *logger;
> + wl_list_for_each(logger, &display->protocol_loggers, link) {
> + if (logger->func == func && logger->user_data == user_data) {
> + wl_list_remove(&logger->link);
> + free(logger);
> + return;
> + }
> + }
> +}
> +
> /** Add support for a wl_shm pixel format
> *
> * \param display The display object
Should wl_display destruction automatically free all loggers, or should
it print a warning if any remain? The very least it should
wl_list_remove() the list head, so that if removal happens by an opaque
pointer like I suggested, it can safely happen after the wl_display is
destroyed.
Looks good to me in general. Again I wish it came with a test.
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/20160617/febff15f/attachment.sig>
More information about the wayland-devel
mailing list