[PATCH weston v3 6/6] Add API to install protocol loggers on the server wl_display

Fri Aug 12 08:25:21 UTC 2016

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.

> >
> > 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.

> >
> > 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
-------------- 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/20160812/da6fc66e/attachment.sig>