[PATCH weston v3 6/6] Add API to install protocol loggers on the server wl_display
Giulio Camuffo
giuliocamuffo at gmail.com
Fri Aug 12 08:31:37 UTC 2016
2016-08-12 10:25 GMT+02:00 Pekka Paalanen <ppaalanen at gmail.com>:
> On Thu, 11 Aug 2016 17:06:02 +0200
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> 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.
>
> Hi,
>
> wl_display is an opaque type, so it works.
>
> If we needed a different wl_protocol_logger_destroy() for each side, it
> wouldn't work. And wl_display_add_protocol_logger() must be different
> for each side since wl_display is different.
>
> I'm thinking about exported symbol conflicts between libwayland-server
> and libwayland-client for a program that links to both.
Ah i get it, you're talking about the functions... You're right, but
on the other hand i doubt a client side version will be needed, and if
it will be i think just adding _client is ok, without needing to add
_server to this one.
>
>> >
>> > 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.
>
> File descriptors are passed on a side-channel, not in the normal data
> flow of a unix socket. I wasn't quite sure if the closure makes that
> difference or not. I think it does not, it should just work. If it did
> make a difference, the value might not be the correct fd.
>
> So I think it's ok, but I'm not absolutely sure.
Sure it's a side-channel, but the fd given to the logger is the same
fd that gets passed to the request handler, so it should be ok as far
as i understand.
>
>
>> >
>> > 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.
>
> Excellent!
>
>
> Thanks,
> pq
More information about the wayland-devel
mailing list