[weston,v5,03/14] libweston: add weston_debug API and implementation

Submitted by Daniel Stone on July 20, 2018, 7:03 p.m.

Details

Message ID 20180720190335.23880-4-daniels@collabora.com
State Superseded
Headers show
Series "weston-debug API and tool" ( rev: 2 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Daniel Stone July 20, 2018, 7:03 p.m.
From: Pekka Paalanen <pq@iki.fi>

weston_debug is both a libweston API for relaying debugging messages,
and the compositor-debug wayland protocol implementation for accessing those
debug messages from a Wayland client.

weston_debug_compositor_{create,destroy}() are private API, hence not
exported.

Signed-off-by: Pekka Paalanen <pq@iki.fi>

append the debug scope name along with the timestamp in
weston_debug_scope_timestamp API

Signed-off-by: Maniraj Devadoss <Maniraj.Devadoss@in.bosch.com>
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

Add explicit advertisement of debug scope names.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 Makefile.am              |   2 +
 libweston/compositor.c   |   6 +
 libweston/compositor.h   |   9 +
 libweston/weston-debug.c | 723 +++++++++++++++++++++++++++++++++++++++
 libweston/weston-debug.h | 107 ++++++
 5 files changed, 847 insertions(+)
 create mode 100644 libweston/weston-debug.c
 create mode 100644 libweston/weston-debug.h

Patch hide | download patch | download mbox

diff --git a/Makefile.am b/Makefile.am
index 66eb365c5..c2d9048b3 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -98,6 +98,8 @@  libweston_@LIBWESTON_MAJOR@_la_SOURCES =			\
 	libweston/linux-dmabuf.h			\
 	libweston/pixel-formats.c			\
 	libweston/pixel-formats.h			\
+	libweston/weston-debug.c			\
+	libweston/weston-debug.h			\
 	shared/helpers.h				\
 	shared/matrix.c					\
 	shared/matrix.h					\
diff --git a/libweston/compositor.c b/libweston/compositor.c
index 9deb7817f..8768f67a0 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -6361,6 +6361,9 @@  weston_compositor_create(struct wl_display *display, void *user_data)
 			      ec, bind_presentation))
 		goto fail;
 
+	if (weston_debug_compositor_create(ec) < 0)
+		goto fail;
+
 	if (weston_input_init(ec) != 0)
 		goto fail;
 
@@ -6699,9 +6702,12 @@  weston_compositor_destroy(struct weston_compositor *compositor)
 
 	weston_plugin_api_destroy_list(compositor);
 
+
 	if (compositor->heads_changed_source)
 		wl_event_source_remove(compositor->heads_changed_source);
 
+	weston_debug_compositor_destroy(compositor);
+
 	free(compositor);
 }
 
diff --git a/libweston/compositor.h b/libweston/compositor.h
index fd0ff7b5b..83448b70f 100644
--- a/libweston/compositor.h
+++ b/libweston/compositor.h
@@ -1049,6 +1049,7 @@  struct weston_touch_calibrator;
 
 struct weston_desktop_xwayland;
 struct weston_desktop_xwayland_interface;
+struct weston_debug_compositor;
 
 struct weston_compositor {
 	struct wl_signal destroy_signal;
@@ -1161,6 +1162,8 @@  struct weston_compositor {
 	weston_touch_calibration_save_func touch_calibration_save;
 	struct weston_layer calibrator_layer;
 	struct weston_touch_calibrator *touch_calibrator;
+
+	struct weston_debug_compositor *weston_debug;
 };
 
 struct weston_buffer {
@@ -2315,6 +2318,12 @@  int
 weston_compositor_enable_touch_calibrator(struct weston_compositor *compositor,
 				weston_touch_calibration_save_func save);
 
+int
+weston_debug_compositor_create(struct weston_compositor *compositor);
+
+void
+weston_debug_compositor_destroy(struct weston_compositor *compositor);
+
 #ifdef  __cplusplus
 }
 #endif
diff --git a/libweston/weston-debug.c b/libweston/weston-debug.c
new file mode 100644
index 000000000..039247f14
--- /dev/null
+++ b/libweston/weston-debug.c
@@ -0,0 +1,723 @@ 
+/*
+ * Copyright © 2017 Pekka Paalanen <pq@iki.fi>
+ *
+ * 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 "config.h"
+
+#include "weston-debug.h"
+#include "helpers.h"
+#include "compositor.h"
+
+#include "weston-debug-server-protocol.h"
+
+#include <unistd.h>
+#include <stdarg.h>
+#include <string.h>
+#include <errno.h>
+#include <sys/time.h>
+
+/** Main weston-debug context
+ *
+ * One per weston_compositor.
+ *
+ * \internal
+ */
+struct weston_debug_compositor {
+	struct weston_compositor *compositor;
+	struct wl_listener compositor_destroy_listener;
+	struct wl_global *global;
+	struct wl_list scope_list; /**< weston_debug_scope::compositor_link */
+
+	struct weston_debug_scope *list;
+};
+
+/** weston-debug message scope
+ *
+ * This is used for scoping debugging messages. Clients can subscribe to
+ * only the scopes they are interested in. A scope is identified by its name
+ * (also referred to as debug stream name).
+ */
+struct weston_debug_scope {
+	char *name;
+	char *desc;
+	weston_debug_scope_cb begin_cb;
+	void *user_data;
+	struct wl_list stream_list; /**< weston_debug_stream::scope_link */
+	struct wl_list compositor_link;
+};
+
+/** A debug stream created by a client
+ *
+ * A client provides a file descriptor for the server to write debug
+ * messages into. A weston_debug_stream is associated to one
+ * weston_debug_scope via the scope name, and the scope provides the messages.
+ * There can be several streams for the same scope, all streams getting the
+ * same messages.
+ */
+struct weston_debug_stream {
+	int fd;				/**< client provided fd */
+	struct wl_resource *resource;	/**< weston_debug_stream_v1 object */
+	struct wl_list scope_link;
+};
+
+static struct weston_debug_scope *
+get_scope(struct weston_debug_compositor *wdc, const char *name)
+{
+	struct weston_debug_scope *scope;
+
+	wl_list_for_each(scope, &wdc->scope_list, compositor_link)
+		if (strcmp(name, scope->name) == 0)
+			return scope;
+
+	return NULL;
+}
+
+static void
+stream_close_unlink(struct weston_debug_stream *stream)
+{
+	if (stream->fd != -1)
+		close(stream->fd);
+	stream->fd = -1;
+
+	wl_list_remove(&stream->scope_link);
+	wl_list_init(&stream->scope_link);
+}
+
+static void WL_PRINTF(2, 3)
+stream_close_on_failure(struct weston_debug_stream *stream,
+			const char *fmt, ...)
+{
+	char *msg;
+	va_list ap;
+	int ret;
+
+	stream_close_unlink(stream);
+
+	va_start(ap, fmt);
+	ret = vasprintf(&msg, fmt, ap);
+	va_end(ap);
+
+	if (ret > 0) {
+		weston_debug_stream_v1_send_failure(stream->resource, msg);
+		free(msg);
+	} else {
+		weston_debug_stream_v1_send_failure(stream->resource,
+						    "MEMFAIL");
+	}
+}
+
+static struct weston_debug_stream *
+stream_create(struct weston_debug_compositor *wdc, const char *name,
+	      int32_t streamfd, struct wl_resource *stream_resource)
+{
+	struct weston_debug_stream *stream;
+	struct weston_debug_scope *scope;
+
+	stream = zalloc(sizeof *stream);
+	if (!stream)
+		return NULL;
+
+	stream->fd = streamfd;
+	stream->resource = stream_resource;
+
+	scope = get_scope(wdc, name);
+	if (scope) {
+		wl_list_insert(&scope->stream_list, &stream->scope_link);
+
+		if (scope->begin_cb)
+			scope->begin_cb(stream, scope->user_data);
+	} else {
+		wl_list_init(&stream->scope_link);
+		stream_close_on_failure(stream,
+					"Debug stream name '%s' is unknown.",
+					name);
+	}
+
+	return stream;
+}
+
+static void
+stream_destroy(struct wl_resource *stream_resource)
+{
+	struct weston_debug_stream *stream;
+
+	stream = wl_resource_get_user_data(stream_resource);
+
+	if (stream->fd != -1)
+		close(stream->fd);
+	wl_list_remove(&stream->scope_link);
+	free(stream);
+}
+
+static void
+weston_debug_stream_destroy(struct wl_client *client,
+			    struct wl_resource *stream_resource)
+{
+	wl_resource_destroy(stream_resource);
+}
+
+static const struct weston_debug_stream_v1_interface
+						weston_debug_stream_impl = {
+	weston_debug_stream_destroy
+};
+
+static void
+weston_debug_destroy(struct wl_client *client,
+		     struct wl_resource *global_resource)
+{
+	wl_resource_destroy(global_resource);
+}
+
+static void
+weston_debug_subscribe(struct wl_client *client,
+		       struct wl_resource *global_resource,
+		       const char *name,
+		       int32_t streamfd,
+		       uint32_t new_stream_id)
+{
+	struct weston_debug_compositor *wdc;
+	struct wl_resource *stream_resource;
+	uint32_t version;
+	struct weston_debug_stream *stream;
+
+	wdc = wl_resource_get_user_data(global_resource);
+	version = wl_resource_get_version(global_resource);
+
+	stream_resource = wl_resource_create(client,
+					&weston_debug_stream_v1_interface,
+					version, new_stream_id);
+	if (!stream_resource)
+		goto fail;
+
+	stream = stream_create(wdc, name, streamfd, stream_resource);
+	if (!stream)
+		goto fail;
+
+	wl_resource_set_implementation(stream_resource,
+				       &weston_debug_stream_impl,
+				       stream, stream_destroy);
+	return;
+
+fail:
+	close(streamfd);
+	wl_client_post_no_memory(client);
+}
+
+static const struct weston_debug_v1_interface weston_debug_impl = {
+	weston_debug_destroy,
+	weston_debug_subscribe
+};
+
+static void
+bind_weston_debug(struct wl_client *client,
+		   void *data, uint32_t version, uint32_t id)
+{
+	struct weston_debug_compositor *wdc = data;
+	struct weston_debug_scope *scope;
+	struct wl_resource *resource;
+
+	resource = wl_resource_create(client,
+				      &weston_debug_v1_interface,
+				      version, id);
+	if (!resource) {
+		wl_client_post_no_memory(client);
+		return;
+	}
+	wl_resource_set_implementation(resource, &weston_debug_impl,
+				       wdc, NULL);
+
+       wl_list_for_each(scope, &wdc->scope_list, compositor_link)
+	       weston_debug_v1_send_available(resource, scope->name);
+}
+
+/** Initialize weston-debug structure
+ *
+ * \param compositor The libweston compositor.
+ * \return 0 on success, -1 on failure.
+ *
+ * weston_debug_compositor is a singleton for each weston_compositor.
+ *
+ * Sets weston_compositor::weston_debug.
+ *
+ * \internal
+ */
+int
+weston_debug_compositor_create(struct weston_compositor *compositor)
+{
+	struct weston_debug_compositor *wdc;
+
+	if (compositor->weston_debug)
+		return -1;
+
+	wdc = zalloc(sizeof *wdc);
+	if (!wdc)
+		return -1;
+
+	wdc->compositor = compositor;
+	wl_list_init(&wdc->scope_list);
+
+	compositor->weston_debug = wdc;
+
+	return 0;
+}
+
+/** Destroy weston_debug_compositor structure
+ *
+ * \param compositor The libweston compositor whose weston-debug to tear down.
+ *
+ * Clears weston_compositor::weston_debug.
+ *
+ * \internal
+ */
+void
+weston_debug_compositor_destroy(struct weston_compositor *compositor)
+{
+	struct weston_debug_compositor *wdc = compositor->weston_debug;
+	struct weston_debug_scope *scope;
+
+	if (wdc->global)
+		wl_global_destroy(wdc->global);
+
+	wl_list_for_each(scope, &wdc->scope_list, compositor_link)
+		weston_log("Internal warning: debug scope '%s' has not been destroyed.\n",
+			   scope->name);
+
+	/* Remove head to not crash if scope removed later. */
+	wl_list_remove(&wdc->scope_list);
+
+	free(wdc);
+
+	compositor->weston_debug = NULL;
+}
+
+/** Enable weston-debug protocol extension
+ *
+ * \param compositor The libweston compositor where to enable.
+ *
+ * This enables the weston_debug_v1 Wayland protocol extension which any
+ * client can use to get debug messsages from the compositor.
+ *
+ * WARNING: This feature should not be used in production. If a client
+ * provides a file descriptor that blocks writes, it will block the whole
+ * compositor indefinitely.
+ *
+ * There is no control on which client is allowed to subscribe to debug
+ * messages. Any and all clients are allowed.
+ *
+ * The debug extension is disabled by default, and once enabled, cannot be
+ * disabled again.
+ */
+WL_EXPORT void
+weston_compositor_enable_debug_protocol(struct weston_compositor *compositor)
+{
+	struct weston_debug_compositor *wdc;
+
+	wdc = compositor->weston_debug;
+	if (!wdc)
+		return;
+
+	if (wdc->global)
+		return;
+
+	wdc->global = wl_global_create(compositor->wl_display,
+				       &weston_debug_v1_interface, 1,
+				       wdc, bind_weston_debug);
+	if (!wdc->global)
+		return;
+
+	weston_log("WARNING: debug protocol has been enabled. "
+		   "This is a potential denial-of-service attack vector and "
+		   "information leak.\n");
+}
+
+/** Register a new debug stream name, creating a debug scope
+ *
+ * \param compositor The libweston compositor where to add.
+ * \param name The debug stream/scope name.
+ * \param desc The debug scope description for humans.
+ * \param begin_cb Optional callback when a client subscribes to this scope.
+ * \param user_data Optional user data pointer for the callback.
+ * \return A valid pointer on success, NULL on failure.
+ *
+ * This function is used to create a debug scope. All debug message printing
+ * happens for a scope, which allows clients to subscribe to the kind of
+ * debug messages they want by \c name.
+ *
+ * \c name must be unique in the \c weston_compositor instance. \c name and
+ * \c description must both be provided. The description is printed when a
+ * client asks for a list of supported debug scopes.
+ *
+ * \c begin_cb, if not NULL, is called when a client subscribes to the
+ * debug scope creating a debug stream. This is for debug scopes that need
+ * to print messages as a response to a client appearing, e.g. printing a
+ * list of windows on demand or a static preamble. The argument \c user_data
+ * is passed in to the callback and is otherwise unused.
+ *
+ * For one-shot debug streams, \c begin_cb should finally call
+ * weston_debug_stream_complete() to close the stream and tell the client
+ * the printing is complete. Otherwise the client expects more to be written
+ * to its file descriptor.
+ *
+ * The debug scope must be destroyed before destroying the
+ * \c weston_compositor.
+ *
+ * \memberof weston_debug_scope
+ * \sa weston_debug_stream, weston_debug_scope_cb
+ */
+WL_EXPORT struct weston_debug_scope *
+weston_compositor_add_debug_scope(struct weston_compositor *compositor,
+				  const char *name,
+				  const char *description,
+				  weston_debug_scope_cb begin_cb,
+				  void *user_data)
+{
+	struct weston_debug_compositor *wdc;
+	struct weston_debug_scope *scope;
+
+	if (!compositor || !name || !description) {
+		weston_log("Error: cannot add a debug scope without name or description.\n");
+		return NULL;
+	}
+
+	wdc = compositor->weston_debug;
+	if (!wdc) {
+		weston_log("Error: cannot add debug scope '%s', infra not initialized.\n",
+			   name);
+		return NULL;
+	}
+
+	if (get_scope(wdc, name)){
+		weston_log("Error: debug scope named '%s' is already registered.\n",
+			   name);
+		return NULL;
+	}
+
+	scope = zalloc(sizeof *scope);
+	if (!scope) {
+		weston_log("Error adding debug scope '%s': out of memory.\n",
+			   name);
+		return NULL;
+	}
+
+	scope->name = strdup(name);
+	scope->desc = strdup(description);
+	scope->begin_cb = begin_cb;
+	scope->user_data = user_data;
+	wl_list_init(&scope->stream_list);
+
+	if (!scope->name || !scope->desc) {
+		weston_log("Error adding debug scope '%s': out of memory.\n",
+			   name);
+		free(scope->name);
+		free(scope->desc);
+		free(scope);
+		return NULL;
+	}
+
+	wl_list_insert(wdc->scope_list.prev, &scope->compositor_link);
+
+	return scope;
+}
+
+/** Destroy a debug scope
+ *
+ * \param scope The debug scope to destroy.
+ *
+ * Destroys the debug scope, closing all open streams subscribed to it and
+ * sending them each a \c weston_debug_stream_v1.failure event.
+ *
+ * \memberof weston_debug_scope
+ */
+WL_EXPORT void
+weston_debug_scope_destroy(struct weston_debug_scope *scope)
+{
+	struct weston_debug_stream *stream;
+
+	if (!scope)
+		return;
+
+	while (!wl_list_empty(&scope->stream_list)) {
+		stream = wl_container_of(scope->stream_list.prev,
+					 stream, scope_link);
+
+		stream_close_on_failure(stream, "debug name removed");
+	}
+
+	wl_list_remove(&scope->compositor_link);
+	free(scope->name);
+	free(scope->desc);
+	free(scope);
+}
+
+/** Are there any active subscriptions to the scope?
+ *
+ * \param scope The debug scope to check.
+ * \return True if any streams are open for this scope, false otherwise.
+ *
+ * As printing some debugging messages may be relatively expensive, one
+ * can use this function to determine if there is a need to gather the
+ * debugging information at all. If this function returns false, all
+ * printing for this scope is dropped, so gathering the information is
+ * pointless.
+ *
+ * The return value of this function should not be stored, as new clients
+ * may subscribe to the debug scope later.
+ *
+ * \memberof weston_debug_scope
+ */
+WL_EXPORT bool
+weston_debug_scope_is_enabled(struct weston_debug_scope *scope)
+{
+	if (!scope)
+		return false;
+
+	return !wl_list_empty(&scope->stream_list);
+}
+
+/** Write data into a specific debug stream
+ *
+ * \param stream The debug stream to write into.
+ * \param data[in] Pointer to the data to write.
+ * \param len Number of bytes to write.
+ *
+ * Writes the given data (binary verbatim) into the debug stream.
+ * If \c len is zero or negative, the write is silently dropped.
+ *
+ * Writing is continued until all data has been written or
+ * a write fails. If the write fails due to a signal, it is re-tried.
+ * Otherwise on failure, the stream is closed and
+ * \c weston_debug_stream_v1.failure event is sent to the client.
+ *
+ * \memberof weston_debug_stream
+ */
+WL_EXPORT void
+weston_debug_stream_write(struct weston_debug_stream *stream,
+			  const char *data, size_t len)
+{
+	ssize_t len_ = len;
+	ssize_t ret;
+	int e;
+
+	if (stream->fd == -1)
+		return;
+
+	while (len_ > 0) {
+		ret = write(stream->fd, data, len_);
+		e = errno;
+		if (ret < 0) {
+			if (e == EINTR)
+				continue;
+
+			stream_close_on_failure(stream,
+					"Error writing %zd bytes: %s (%d)",
+					len_, strerror(e), e);
+			break;
+		}
+
+		len_ -= ret;
+		data += ret;
+	}
+}
+
+/** Write a formatted string into a specific debug stream (varargs)
+ *
+ * \param stream The debug stream to write into.
+ * \param fmt Printf-style format string.
+ * \param ap Formatting arguments.
+ *
+ * The behavioral details are the same as for weston_debug_stream_write().
+ *
+ * \memberof weston_debug_stream
+ */
+WL_EXPORT void
+weston_debug_stream_vprintf(struct weston_debug_stream *stream,
+			    const char *fmt, va_list ap)
+{
+	char *str;
+	int len;
+
+	len = vasprintf(&str, fmt, ap);
+	if (len >= 0) {
+		weston_debug_stream_write(stream, str, len);
+		free(str);
+	} else {
+		stream_close_on_failure(stream, "Out of memory");
+	}
+}
+
+/** Write a formatted string into a specific debug stream
+ *
+ * \param stream The debug stream to write into.
+ * \param fmt Printf-style format string and arguments.
+ *
+ * The behavioral details are the same as for weston_debug_stream_write().
+ *
+ * \memberof weston_debug_stream
+ */
+WL_EXPORT void
+weston_debug_stream_printf(struct weston_debug_stream *stream,
+			   const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	weston_debug_stream_vprintf(stream, fmt, ap);
+	va_end(ap);
+}
+
+/** Close the debug stream and send success event
+ *
+ * \param stream The debug stream to close.
+ *
+ * Closes the debug stream and sends \c weston_debug_stream_v1.complete
+ * event to the client. This tells the client the debug information dump
+ * is complete.
+ *
+ * \memberof weston_debug_stream
+ */
+WL_EXPORT void
+weston_debug_stream_complete(struct weston_debug_stream *stream)
+{
+	stream_close_unlink(stream);
+	weston_debug_stream_v1_send_complete(stream->resource);
+}
+
+/** Write debug data for a scope
+ *
+ * \param scope The debug scope to write for.
+ * \param data[in] Pointer to the data to write.
+ * \param len Number of bytes to write.
+ *
+ * Writes the given data to all subscribed clients' streams.
+ *
+ * The behavioral details for each stream are the same as for
+ * weston_debug_stream_write().
+ *
+ * \memberof weston_debug_scope
+ */
+WL_EXPORT void
+weston_debug_scope_write(struct weston_debug_scope *scope,
+			 const char *data, size_t len)
+{
+	struct weston_debug_stream *stream;
+
+	if (!scope)
+		return;
+
+	wl_list_for_each(stream, &scope->stream_list, scope_link)
+		weston_debug_stream_write(stream, data, len);
+}
+
+/** Write a formatted string for a scope (varargs)
+ *
+ * \param scope The debug scope to write for.
+ * \param fmt Printf-style format string.
+ * \param ap Formatting arguments.
+ *
+ * Writes to formatted string to all subscribed clients' streams.
+ *
+ * The behavioral details for each stream are the same as for
+ * weston_debug_stream_write().
+ *
+ * \memberof weston_debug_scope
+ */
+WL_EXPORT void
+weston_debug_scope_vprintf(struct weston_debug_scope *scope,
+			   const char *fmt, va_list ap)
+{
+	static const char oom[] = "Out of memory";
+	char *str;
+	int len;
+
+	if (!weston_debug_scope_is_enabled(scope))
+		return;
+
+	len = vasprintf(&str, fmt, ap);
+	if (len >= 0) {
+		weston_debug_scope_write(scope, str, len);
+		free(str);
+	} else {
+		weston_debug_scope_write(scope, oom, sizeof oom - 1);
+	}
+}
+
+/** Write a formatted string for a scope
+ *
+ * \param scope The debug scope to write for.
+ * \param fmt Printf-style format string and arguments.
+ *
+ * Writes to formatted string to all subscribed clients' streams.
+ *
+ * The behavioral details for each stream are the same as for
+ * weston_debug_stream_write().
+ *
+ * \memberof weston_debug_scope
+ */
+WL_EXPORT void
+weston_debug_scope_printf(struct weston_debug_scope *scope,
+			  const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	weston_debug_scope_vprintf(scope, fmt, ap);
+	va_end(ap);
+}
+
+/** Format current time as a string
+ * and append the debug scope name to it
+ *
+ * \param scope[in] debug scope.
+ * \param buf[out] Buffer to store the string.
+ * \param len Available size in the buffer in bytes.
+ * \return \c buf
+ *
+ * Reads the current local wall-clock time and formats it into a string.
+ * and append the debug scope name to it.
+ * The string is nul-terminated, even if truncated.
+ */
+WL_EXPORT char *
+weston_debug_scope_timestamp(struct weston_debug_scope *scope,
+			     char *buf, size_t len)
+{
+	struct timeval tv;
+	struct tm *bdt;
+	char string[128];
+	size_t ret = 0;
+
+	gettimeofday(&tv, NULL);
+
+	bdt = localtime(&tv.tv_sec);
+	if (bdt)
+		ret = strftime(string, sizeof string,
+			       "%Y-%m-%d %H:%M:%S", bdt);
+
+	if (ret > 0)
+		snprintf(buf, len, "[%s.%03ld][%s]", string,
+			 tv.tv_usec / 1000, scope->name);
+	else
+		snprintf(buf, len, "[?][%s]", scope->name);
+
+	return buf;
+}
diff --git a/libweston/weston-debug.h b/libweston/weston-debug.h
new file mode 100644
index 000000000..c76cec852
--- /dev/null
+++ b/libweston/weston-debug.h
@@ -0,0 +1,107 @@ 
+/*
+ * Copyright © 2017 Pekka Paalanen <pq@iki.fi>
+ *
+ * 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.
+ */
+
+#ifndef WESTON_DEBUG_H
+#define WESTON_DEBUG_H
+
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdarg.h>
+
+#ifdef  __cplusplus
+extern "C" {
+#endif
+
+struct weston_compositor;
+
+void
+weston_compositor_enable_debug_protocol(struct weston_compositor *);
+
+struct weston_debug_scope;
+struct weston_debug_stream;
+
+/** weston_debug_scope callback
+ *
+ * \param stream The debug stream.
+ * \param user_data The \c user_data argument given to
+ * weston_compositor_add_debug_scope()
+ *
+ * \memberof weston_debug_scope
+ * \sa weston_debug_stream
+ */
+typedef void (*weston_debug_scope_cb)(struct weston_debug_stream *stream,
+				      void *user_data);
+
+struct weston_debug_scope *
+weston_compositor_add_debug_scope(struct weston_compositor *compositor,
+				  const char *name,
+				  const char *description,
+				  weston_debug_scope_cb begin_cb,
+				  void *user_data);
+
+void
+weston_debug_scope_destroy(struct weston_debug_scope *scope);
+
+bool
+weston_debug_scope_is_enabled(struct weston_debug_scope *scope);
+
+void
+weston_debug_scope_write(struct weston_debug_scope *scope,
+			 const char *data, size_t len);
+
+void
+weston_debug_scope_vprintf(struct weston_debug_scope *scope,
+			   const char *fmt, va_list ap);
+
+void
+weston_debug_scope_printf(struct weston_debug_scope *scope,
+			  const char *fmt, ...)
+			  __attribute__ ((format (printf, 2, 3)));
+
+void
+weston_debug_stream_write(struct weston_debug_stream *stream,
+			  const char *data, size_t len);
+
+void
+weston_debug_stream_vprintf(struct weston_debug_stream *stream,
+			    const char *fmt, va_list ap);
+
+void
+weston_debug_stream_printf(struct weston_debug_stream *stream,
+			   const char *fmt, ...)
+			   __attribute__ ((format (printf, 2, 3)));
+
+void
+weston_debug_stream_complete(struct weston_debug_stream *stream);
+
+char *
+weston_debug_scope_timestamp(struct weston_debug_scope *scope,
+			     char *buf, size_t len);
+
+#ifdef  __cplusplus
+}
+#endif
+
+#endif /* WESTON_DEBUG_H */

Comments

Reviewed-by: Emre Ucan <eucan@de.adit-jv.com>


Best regards

Emre Ucan
Engineering Software Base (ADITG/ESB)

Tel. +49 5121 49 6937

> -----Original Message-----

> From: wayland-devel [mailto:wayland-devel-

> bounces@lists.freedesktop.org] On Behalf Of Daniel Stone

> Sent: Freitag, 20. Juli 2018 21:06

> To: wayland-devel@lists.freedesktop.org

> Cc: pekka.paalanen@collabora.co.uk; emre.ucan@de.adit-jv.com;

> Maniraj.Devadoss@in.bosch.com

> Subject: [PATCH weston v5 03/14] libweston: add weston_debug API and

> implementation

> 

> From: Pekka Paalanen <pq@iki.fi>

> 

> weston_debug is both a libweston API for relaying debugging messages,

> and the compositor-debug wayland protocol implementation for accessing

> those

> debug messages from a Wayland client.

> 

> weston_debug_compositor_{create,destroy}() are private API, hence not

> exported.

> 

> Signed-off-by: Pekka Paalanen <pq@iki.fi>

> 

> append the debug scope name along with the timestamp in

> weston_debug_scope_timestamp API

> 

> Signed-off-by: Maniraj Devadoss <Maniraj.Devadoss@in.bosch.com>

> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

> 

> Add explicit advertisement of debug scope names.

> 

> Signed-off-by: Daniel Stone <daniels@collabora.com>

> ---

>  Makefile.am              |   2 +

>  libweston/compositor.c   |   6 +

>  libweston/compositor.h   |   9 +

>  libweston/weston-debug.c | 723

> +++++++++++++++++++++++++++++++++++++++

>  libweston/weston-debug.h | 107 ++++++

>  5 files changed, 847 insertions(+)

>  create mode 100644 libweston/weston-debug.c

>  create mode 100644 libweston/weston-debug.h

> 

> diff --git a/Makefile.am b/Makefile.am

> index 66eb365c5..c2d9048b3 100644

> --- a/Makefile.am

> +++ b/Makefile.am

> @@ -98,6 +98,8 @@ libweston_@LIBWESTON_MAJOR@_la_SOURCES =

> 			\

>  	libweston/linux-dmabuf.h			\

>  	libweston/pixel-formats.c			\

>  	libweston/pixel-formats.h			\

> +	libweston/weston-debug.c			\

> +	libweston/weston-debug.h			\

>  	shared/helpers.h				\

>  	shared/matrix.c					\

>  	shared/matrix.h					\

> diff --git a/libweston/compositor.c b/libweston/compositor.c

> index 9deb7817f..8768f67a0 100644

> --- a/libweston/compositor.c

> +++ b/libweston/compositor.c

> @@ -6361,6 +6361,9 @@ weston_compositor_create(struct wl_display

> *display, void *user_data)

>  			      ec, bind_presentation))

>  		goto fail;

> 

> +	if (weston_debug_compositor_create(ec) < 0)

> +		goto fail;

> +

>  	if (weston_input_init(ec) != 0)

>  		goto fail;

> 

> @@ -6699,9 +6702,12 @@ weston_compositor_destroy(struct

> weston_compositor *compositor)

> 

>  	weston_plugin_api_destroy_list(compositor);

> 

> +

>  	if (compositor->heads_changed_source)

>  		wl_event_source_remove(compositor-

> >heads_changed_source);

> 

> +	weston_debug_compositor_destroy(compositor);

> +

>  	free(compositor);

>  }

> 

> diff --git a/libweston/compositor.h b/libweston/compositor.h

> index fd0ff7b5b..83448b70f 100644

> --- a/libweston/compositor.h

> +++ b/libweston/compositor.h

> @@ -1049,6 +1049,7 @@ struct weston_touch_calibrator;

> 

>  struct weston_desktop_xwayland;

>  struct weston_desktop_xwayland_interface;

> +struct weston_debug_compositor;

> 

>  struct weston_compositor {

>  	struct wl_signal destroy_signal;

> @@ -1161,6 +1162,8 @@ struct weston_compositor {

>  	weston_touch_calibration_save_func touch_calibration_save;

>  	struct weston_layer calibrator_layer;

>  	struct weston_touch_calibrator *touch_calibrator;

> +

> +	struct weston_debug_compositor *weston_debug;

>  };

> 

>  struct weston_buffer {

> @@ -2315,6 +2318,12 @@ int

>  weston_compositor_enable_touch_calibrator(struct weston_compositor

> *compositor,

>  				weston_touch_calibration_save_func save);

> 

> +int

> +weston_debug_compositor_create(struct weston_compositor

> *compositor);

> +

> +void

> +weston_debug_compositor_destroy(struct weston_compositor

> *compositor);

> +

>  #ifdef  __cplusplus

>  }

>  #endif

> diff --git a/libweston/weston-debug.c b/libweston/weston-debug.c

> new file mode 100644

> index 000000000..039247f14

> --- /dev/null

> +++ b/libweston/weston-debug.c

> @@ -0,0 +1,723 @@

> +/*

> + * Copyright © 2017 Pekka Paalanen <pq@iki.fi>

> + *

> + * 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 "config.h"

> +

> +#include "weston-debug.h"

> +#include "helpers.h"

> +#include "compositor.h"

> +

> +#include "weston-debug-server-protocol.h"

> +

> +#include <unistd.h>

> +#include <stdarg.h>

> +#include <string.h>

> +#include <errno.h>

> +#include <sys/time.h>

> +

> +/** Main weston-debug context

> + *

> + * One per weston_compositor.

> + *

> + * \internal

> + */

> +struct weston_debug_compositor {

> +	struct weston_compositor *compositor;

> +	struct wl_listener compositor_destroy_listener;

> +	struct wl_global *global;

> +	struct wl_list scope_list; /**<

> weston_debug_scope::compositor_link */

> +

> +	struct weston_debug_scope *list;

> +};

> +

> +/** weston-debug message scope

> + *

> + * This is used for scoping debugging messages. Clients can subscribe to

> + * only the scopes they are interested in. A scope is identified by its name

> + * (also referred to as debug stream name).

> + */

> +struct weston_debug_scope {

> +	char *name;

> +	char *desc;

> +	weston_debug_scope_cb begin_cb;

> +	void *user_data;

> +	struct wl_list stream_list; /**< weston_debug_stream::scope_link */

> +	struct wl_list compositor_link;

> +};

> +

> +/** A debug stream created by a client

> + *

> + * A client provides a file descriptor for the server to write debug

> + * messages into. A weston_debug_stream is associated to one

> + * weston_debug_scope via the scope name, and the scope provides the

> messages.

> + * There can be several streams for the same scope, all streams getting the

> + * same messages.

> + */

> +struct weston_debug_stream {

> +	int fd;				/**< client provided fd */

> +	struct wl_resource *resource;	/**< weston_debug_stream_v1

> object */

> +	struct wl_list scope_link;

> +};

> +

> +static struct weston_debug_scope *

> +get_scope(struct weston_debug_compositor *wdc, const char *name)

> +{

> +	struct weston_debug_scope *scope;

> +

> +	wl_list_for_each(scope, &wdc->scope_list, compositor_link)

> +		if (strcmp(name, scope->name) == 0)

> +			return scope;

> +

> +	return NULL;

> +}

> +

> +static void

> +stream_close_unlink(struct weston_debug_stream *stream)

> +{

> +	if (stream->fd != -1)

> +		close(stream->fd);

> +	stream->fd = -1;

> +

> +	wl_list_remove(&stream->scope_link);

> +	wl_list_init(&stream->scope_link);

> +}

> +

> +static void WL_PRINTF(2, 3)

> +stream_close_on_failure(struct weston_debug_stream *stream,

> +			const char *fmt, ...)

> +{

> +	char *msg;

> +	va_list ap;

> +	int ret;

> +

> +	stream_close_unlink(stream);

> +

> +	va_start(ap, fmt);

> +	ret = vasprintf(&msg, fmt, ap);

> +	va_end(ap);

> +

> +	if (ret > 0) {

> +		weston_debug_stream_v1_send_failure(stream->resource,

> msg);

> +		free(msg);

> +	} else {

> +		weston_debug_stream_v1_send_failure(stream->resource,

> +						    "MEMFAIL");

> +	}

> +}

> +

> +static struct weston_debug_stream *

> +stream_create(struct weston_debug_compositor *wdc, const char *name,

> +	      int32_t streamfd, struct wl_resource *stream_resource)

> +{

> +	struct weston_debug_stream *stream;

> +	struct weston_debug_scope *scope;

> +

> +	stream = zalloc(sizeof *stream);

> +	if (!stream)

> +		return NULL;

> +

> +	stream->fd = streamfd;

> +	stream->resource = stream_resource;

> +

> +	scope = get_scope(wdc, name);

> +	if (scope) {

> +		wl_list_insert(&scope->stream_list, &stream->scope_link);

> +

> +		if (scope->begin_cb)

> +			scope->begin_cb(stream, scope->user_data);

> +	} else {

> +		wl_list_init(&stream->scope_link);

> +		stream_close_on_failure(stream,

> +					"Debug stream name '%s' is

> unknown.",

> +					name);

> +	}

> +

> +	return stream;

> +}

> +

> +static void

> +stream_destroy(struct wl_resource *stream_resource)

> +{

> +	struct weston_debug_stream *stream;

> +

> +	stream = wl_resource_get_user_data(stream_resource);

> +

> +	if (stream->fd != -1)

> +		close(stream->fd);

> +	wl_list_remove(&stream->scope_link);

> +	free(stream);

> +}

> +

> +static void

> +weston_debug_stream_destroy(struct wl_client *client,

> +			    struct wl_resource *stream_resource)

> +{

> +	wl_resource_destroy(stream_resource);

> +}

> +

> +static const struct weston_debug_stream_v1_interface

> +						weston_debug_stream_impl

> = {

> +	weston_debug_stream_destroy

> +};

> +

> +static void

> +weston_debug_destroy(struct wl_client *client,

> +		     struct wl_resource *global_resource)

> +{

> +	wl_resource_destroy(global_resource);

> +}

> +

> +static void

> +weston_debug_subscribe(struct wl_client *client,

> +		       struct wl_resource *global_resource,

> +		       const char *name,

> +		       int32_t streamfd,

> +		       uint32_t new_stream_id)

> +{

> +	struct weston_debug_compositor *wdc;

> +	struct wl_resource *stream_resource;

> +	uint32_t version;

> +	struct weston_debug_stream *stream;

> +

> +	wdc = wl_resource_get_user_data(global_resource);

> +	version = wl_resource_get_version(global_resource);

> +

> +	stream_resource = wl_resource_create(client,

> +

> 	&weston_debug_stream_v1_interface,

> +					version, new_stream_id);

> +	if (!stream_resource)

> +		goto fail;

> +

> +	stream = stream_create(wdc, name, streamfd, stream_resource);

> +	if (!stream)

> +		goto fail;

> +

> +	wl_resource_set_implementation(stream_resource,

> +				       &weston_debug_stream_impl,

> +				       stream, stream_destroy);

> +	return;

> +

> +fail:

> +	close(streamfd);

> +	wl_client_post_no_memory(client);

> +}

> +

> +static const struct weston_debug_v1_interface weston_debug_impl = {

> +	weston_debug_destroy,

> +	weston_debug_subscribe

> +};

> +

> +static void

> +bind_weston_debug(struct wl_client *client,

> +		   void *data, uint32_t version, uint32_t id)

> +{

> +	struct weston_debug_compositor *wdc = data;

> +	struct weston_debug_scope *scope;

> +	struct wl_resource *resource;

> +

> +	resource = wl_resource_create(client,

> +				      &weston_debug_v1_interface,

> +				      version, id);

> +	if (!resource) {

> +		wl_client_post_no_memory(client);

> +		return;

> +	}

> +	wl_resource_set_implementation(resource, &weston_debug_impl,

> +				       wdc, NULL);

> +

> +       wl_list_for_each(scope, &wdc->scope_list, compositor_link)

> +	       weston_debug_v1_send_available(resource, scope->name);

> +}

> +

> +/** Initialize weston-debug structure

> + *

> + * \param compositor The libweston compositor.

> + * \return 0 on success, -1 on failure.

> + *

> + * weston_debug_compositor is a singleton for each weston_compositor.

> + *

> + * Sets weston_compositor::weston_debug.

> + *

> + * \internal

> + */

> +int

> +weston_debug_compositor_create(struct weston_compositor

> *compositor)

> +{

> +	struct weston_debug_compositor *wdc;

> +

> +	if (compositor->weston_debug)

> +		return -1;

> +

> +	wdc = zalloc(sizeof *wdc);

> +	if (!wdc)

> +		return -1;

> +

> +	wdc->compositor = compositor;

> +	wl_list_init(&wdc->scope_list);

> +

> +	compositor->weston_debug = wdc;

> +

> +	return 0;

> +}

> +

> +/** Destroy weston_debug_compositor structure

> + *

> + * \param compositor The libweston compositor whose weston-debug to

> tear down.

> + *

> + * Clears weston_compositor::weston_debug.

> + *

> + * \internal

> + */

> +void

> +weston_debug_compositor_destroy(struct weston_compositor

> *compositor)

> +{

> +	struct weston_debug_compositor *wdc = compositor-

> >weston_debug;

> +	struct weston_debug_scope *scope;

> +

> +	if (wdc->global)

> +		wl_global_destroy(wdc->global);

> +

> +	wl_list_for_each(scope, &wdc->scope_list, compositor_link)

> +		weston_log("Internal warning: debug scope '%s' has not

> been destroyed.\n",

> +			   scope->name);

> +

> +	/* Remove head to not crash if scope removed later. */

> +	wl_list_remove(&wdc->scope_list);

> +

> +	free(wdc);

> +

> +	compositor->weston_debug = NULL;

> +}

> +

> +/** Enable weston-debug protocol extension

> + *

> + * \param compositor The libweston compositor where to enable.

> + *

> + * This enables the weston_debug_v1 Wayland protocol extension which

> any

> + * client can use to get debug messsages from the compositor.

> + *

> + * WARNING: This feature should not be used in production. If a client

> + * provides a file descriptor that blocks writes, it will block the whole

> + * compositor indefinitely.

> + *

> + * There is no control on which client is allowed to subscribe to debug

> + * messages. Any and all clients are allowed.

> + *

> + * The debug extension is disabled by default, and once enabled, cannot be

> + * disabled again.

> + */

> +WL_EXPORT void

> +weston_compositor_enable_debug_protocol(struct weston_compositor

> *compositor)

> +{

> +	struct weston_debug_compositor *wdc;

> +

> +	wdc = compositor->weston_debug;

> +	if (!wdc)

> +		return;

> +

> +	if (wdc->global)

> +		return;

> +

> +	wdc->global = wl_global_create(compositor->wl_display,

> +				       &weston_debug_v1_interface, 1,

> +				       wdc, bind_weston_debug);

> +	if (!wdc->global)

> +		return;

> +

> +	weston_log("WARNING: debug protocol has been enabled. "

> +		   "This is a potential denial-of-service attack vector and "

> +		   "information leak.\n");

> +}

> +

> +/** Register a new debug stream name, creating a debug scope

> + *

> + * \param compositor The libweston compositor where to add.

> + * \param name The debug stream/scope name.

> + * \param desc The debug scope description for humans.

> + * \param begin_cb Optional callback when a client subscribes to this scope.

> + * \param user_data Optional user data pointer for the callback.

> + * \return A valid pointer on success, NULL on failure.

> + *

> + * This function is used to create a debug scope. All debug message printing

> + * happens for a scope, which allows clients to subscribe to the kind of

> + * debug messages they want by \c name.

> + *

> + * \c name must be unique in the \c weston_compositor instance. \c name

> and

> + * \c description must both be provided. The description is printed when a

> + * client asks for a list of supported debug scopes.

> + *

> + * \c begin_cb, if not NULL, is called when a client subscribes to the

> + * debug scope creating a debug stream. This is for debug scopes that need

> + * to print messages as a response to a client appearing, e.g. printing a

> + * list of windows on demand or a static preamble. The argument \c

> user_data

> + * is passed in to the callback and is otherwise unused.

> + *

> + * For one-shot debug streams, \c begin_cb should finally call

> + * weston_debug_stream_complete() to close the stream and tell the client

> + * the printing is complete. Otherwise the client expects more to be written

> + * to its file descriptor.

> + *

> + * The debug scope must be destroyed before destroying the

> + * \c weston_compositor.

> + *

> + * \memberof weston_debug_scope

> + * \sa weston_debug_stream, weston_debug_scope_cb

> + */

> +WL_EXPORT struct weston_debug_scope *

> +weston_compositor_add_debug_scope(struct weston_compositor

> *compositor,

> +				  const char *name,

> +				  const char *description,

> +				  weston_debug_scope_cb begin_cb,

> +				  void *user_data)

> +{

> +	struct weston_debug_compositor *wdc;

> +	struct weston_debug_scope *scope;

> +

> +	if (!compositor || !name || !description) {

> +		weston_log("Error: cannot add a debug scope without name

> or description.\n");

> +		return NULL;

> +	}

> +

> +	wdc = compositor->weston_debug;

> +	if (!wdc) {

> +		weston_log("Error: cannot add debug scope '%s', infra not

> initialized.\n",

> +			   name);

> +		return NULL;

> +	}

> +

> +	if (get_scope(wdc, name)){

> +		weston_log("Error: debug scope named '%s' is already

> registered.\n",

> +			   name);

> +		return NULL;

> +	}

> +

> +	scope = zalloc(sizeof *scope);

> +	if (!scope) {

> +		weston_log("Error adding debug scope '%s': out of

> memory.\n",

> +			   name);

> +		return NULL;

> +	}

> +

> +	scope->name = strdup(name);

> +	scope->desc = strdup(description);

> +	scope->begin_cb = begin_cb;

> +	scope->user_data = user_data;

> +	wl_list_init(&scope->stream_list);

> +

> +	if (!scope->name || !scope->desc) {

> +		weston_log("Error adding debug scope '%s': out of

> memory.\n",

> +			   name);

> +		free(scope->name);

> +		free(scope->desc);

> +		free(scope);

> +		return NULL;

> +	}

> +

> +	wl_list_insert(wdc->scope_list.prev, &scope->compositor_link);

> +

> +	return scope;

> +}

> +

> +/** Destroy a debug scope

> + *

> + * \param scope The debug scope to destroy.

> + *

> + * Destroys the debug scope, closing all open streams subscribed to it and

> + * sending them each a \c weston_debug_stream_v1.failure event.

> + *

> + * \memberof weston_debug_scope

> + */

> +WL_EXPORT void

> +weston_debug_scope_destroy(struct weston_debug_scope *scope)

> +{

> +	struct weston_debug_stream *stream;

> +

> +	if (!scope)

> +		return;

> +

> +	while (!wl_list_empty(&scope->stream_list)) {

> +		stream = wl_container_of(scope->stream_list.prev,

> +					 stream, scope_link);

> +

> +		stream_close_on_failure(stream, "debug name removed");

> +	}

> +

> +	wl_list_remove(&scope->compositor_link);

> +	free(scope->name);

> +	free(scope->desc);

> +	free(scope);

> +}

> +

> +/** Are there any active subscriptions to the scope?

> + *

> + * \param scope The debug scope to check.

> + * \return True if any streams are open for this scope, false otherwise.

> + *

> + * As printing some debugging messages may be relatively expensive, one

> + * can use this function to determine if there is a need to gather the

> + * debugging information at all. If this function returns false, all

> + * printing for this scope is dropped, so gathering the information is

> + * pointless.

> + *

> + * The return value of this function should not be stored, as new clients

> + * may subscribe to the debug scope later.

> + *

> + * \memberof weston_debug_scope

> + */

> +WL_EXPORT bool

> +weston_debug_scope_is_enabled(struct weston_debug_scope *scope)

> +{

> +	if (!scope)

> +		return false;

> +

> +	return !wl_list_empty(&scope->stream_list);

> +}

> +

> +/** Write data into a specific debug stream

> + *

> + * \param stream The debug stream to write into.

> + * \param data[in] Pointer to the data to write.

> + * \param len Number of bytes to write.

> + *

> + * Writes the given data (binary verbatim) into the debug stream.

> + * If \c len is zero or negative, the write is silently dropped.

> + *

> + * Writing is continued until all data has been written or

> + * a write fails. If the write fails due to a signal, it is re-tried.

> + * Otherwise on failure, the stream is closed and

> + * \c weston_debug_stream_v1.failure event is sent to the client.

> + *

> + * \memberof weston_debug_stream

> + */

> +WL_EXPORT void

> +weston_debug_stream_write(struct weston_debug_stream *stream,

> +			  const char *data, size_t len)

> +{

> +	ssize_t len_ = len;

> +	ssize_t ret;

> +	int e;

> +

> +	if (stream->fd == -1)

> +		return;

> +

> +	while (len_ > 0) {

> +		ret = write(stream->fd, data, len_);

> +		e = errno;

> +		if (ret < 0) {

> +			if (e == EINTR)

> +				continue;

> +

> +			stream_close_on_failure(stream,

> +					"Error writing %zd bytes: %s (%d)",

> +					len_, strerror(e), e);

> +			break;

> +		}

> +

> +		len_ -= ret;

> +		data += ret;

> +	}

> +}

> +

> +/** Write a formatted string into a specific debug stream (varargs)

> + *

> + * \param stream The debug stream to write into.

> + * \param fmt Printf-style format string.

> + * \param ap Formatting arguments.

> + *

> + * The behavioral details are the same as for

> weston_debug_stream_write().

> + *

> + * \memberof weston_debug_stream

> + */

> +WL_EXPORT void

> +weston_debug_stream_vprintf(struct weston_debug_stream *stream,

> +			    const char *fmt, va_list ap)

> +{

> +	char *str;

> +	int len;

> +

> +	len = vasprintf(&str, fmt, ap);

> +	if (len >= 0) {

> +		weston_debug_stream_write(stream, str, len);

> +		free(str);

> +	} else {

> +		stream_close_on_failure(stream, "Out of memory");

> +	}

> +}

> +

> +/** Write a formatted string into a specific debug stream

> + *

> + * \param stream The debug stream to write into.

> + * \param fmt Printf-style format string and arguments.

> + *

> + * The behavioral details are the same as for

> weston_debug_stream_write().

> + *

> + * \memberof weston_debug_stream

> + */

> +WL_EXPORT void

> +weston_debug_stream_printf(struct weston_debug_stream *stream,

> +			   const char *fmt, ...)

> +{

> +	va_list ap;

> +

> +	va_start(ap, fmt);

> +	weston_debug_stream_vprintf(stream, fmt, ap);

> +	va_end(ap);

> +}

> +

> +/** Close the debug stream and send success event

> + *

> + * \param stream The debug stream to close.

> + *

> + * Closes the debug stream and sends \c

> weston_debug_stream_v1.complete

> + * event to the client. This tells the client the debug information dump

> + * is complete.

> + *

> + * \memberof weston_debug_stream

> + */

> +WL_EXPORT void

> +weston_debug_stream_complete(struct weston_debug_stream *stream)

> +{

> +	stream_close_unlink(stream);

> +	weston_debug_stream_v1_send_complete(stream->resource);

> +}

> +

> +/** Write debug data for a scope

> + *

> + * \param scope The debug scope to write for.

> + * \param data[in] Pointer to the data to write.

> + * \param len Number of bytes to write.

> + *

> + * Writes the given data to all subscribed clients' streams.

> + *

> + * The behavioral details for each stream are the same as for

> + * weston_debug_stream_write().

> + *

> + * \memberof weston_debug_scope

> + */

> +WL_EXPORT void

> +weston_debug_scope_write(struct weston_debug_scope *scope,

> +			 const char *data, size_t len)

> +{

> +	struct weston_debug_stream *stream;

> +

> +	if (!scope)

> +		return;

> +

> +	wl_list_for_each(stream, &scope->stream_list, scope_link)

> +		weston_debug_stream_write(stream, data, len);

> +}

> +

> +/** Write a formatted string for a scope (varargs)

> + *

> + * \param scope The debug scope to write for.

> + * \param fmt Printf-style format string.

> + * \param ap Formatting arguments.

> + *

> + * Writes to formatted string to all subscribed clients' streams.

> + *

> + * The behavioral details for each stream are the same as for

> + * weston_debug_stream_write().

> + *

> + * \memberof weston_debug_scope

> + */

> +WL_EXPORT void

> +weston_debug_scope_vprintf(struct weston_debug_scope *scope,

> +			   const char *fmt, va_list ap)

> +{

> +	static const char oom[] = "Out of memory";

> +	char *str;

> +	int len;

> +

> +	if (!weston_debug_scope_is_enabled(scope))

> +		return;

> +

> +	len = vasprintf(&str, fmt, ap);

> +	if (len >= 0) {

> +		weston_debug_scope_write(scope, str, len);

> +		free(str);

> +	} else {

> +		weston_debug_scope_write(scope, oom, sizeof oom - 1);

> +	}

> +}

> +

> +/** Write a formatted string for a scope

> + *

> + * \param scope The debug scope to write for.

> + * \param fmt Printf-style format string and arguments.

> + *

> + * Writes to formatted string to all subscribed clients' streams.

> + *

> + * The behavioral details for each stream are the same as for

> + * weston_debug_stream_write().

> + *

> + * \memberof weston_debug_scope

> + */

> +WL_EXPORT void

> +weston_debug_scope_printf(struct weston_debug_scope *scope,

> +			  const char *fmt, ...)

> +{

> +	va_list ap;

> +

> +	va_start(ap, fmt);

> +	weston_debug_scope_vprintf(scope, fmt, ap);

> +	va_end(ap);

> +}

> +

> +/** Format current time as a string

> + * and append the debug scope name to it

> + *

> + * \param scope[in] debug scope.

> + * \param buf[out] Buffer to store the string.

> + * \param len Available size in the buffer in bytes.

> + * \return \c buf

> + *

> + * Reads the current local wall-clock time and formats it into a string.

> + * and append the debug scope name to it.

> + * The string is nul-terminated, even if truncated.

> + */

> +WL_EXPORT char *

> +weston_debug_scope_timestamp(struct weston_debug_scope *scope,

> +			     char *buf, size_t len)

> +{

> +	struct timeval tv;

> +	struct tm *bdt;

> +	char string[128];

> +	size_t ret = 0;

> +

> +	gettimeofday(&tv, NULL);

> +

> +	bdt = localtime(&tv.tv_sec);

> +	if (bdt)

> +		ret = strftime(string, sizeof string,

> +			       "%Y-%m-%d %H:%M:%S", bdt);

> +

> +	if (ret > 0)

> +		snprintf(buf, len, "[%s.%03ld][%s]", string,

> +			 tv.tv_usec / 1000, scope->name);

> +	else

> +		snprintf(buf, len, "[?][%s]", scope->name);

> +

> +	return buf;

> +}

> diff --git a/libweston/weston-debug.h b/libweston/weston-debug.h

> new file mode 100644

> index 000000000..c76cec852

> --- /dev/null

> +++ b/libweston/weston-debug.h

> @@ -0,0 +1,107 @@

> +/*

> + * Copyright © 2017 Pekka Paalanen <pq@iki.fi>

> + *

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

> + */

> +

> +#ifndef WESTON_DEBUG_H

> +#define WESTON_DEBUG_H

> +

> +#include <stdbool.h>

> +#include <stdlib.h>

> +#include <stdarg.h>

> +

> +#ifdef  __cplusplus

> +extern "C" {

> +#endif

> +

> +struct weston_compositor;

> +

> +void

> +weston_compositor_enable_debug_protocol(struct weston_compositor

> *);

> +

> +struct weston_debug_scope;

> +struct weston_debug_stream;

> +

> +/** weston_debug_scope callback

> + *

> + * \param stream The debug stream.

> + * \param user_data The \c user_data argument given to

> + * weston_compositor_add_debug_scope()

> + *

> + * \memberof weston_debug_scope

> + * \sa weston_debug_stream

> + */

> +typedef void (*weston_debug_scope_cb)(struct weston_debug_stream

> *stream,

> +				      void *user_data);

> +

> +struct weston_debug_scope *

> +weston_compositor_add_debug_scope(struct weston_compositor

> *compositor,

> +				  const char *name,

> +				  const char *description,

> +				  weston_debug_scope_cb begin_cb,

> +				  void *user_data);

> +

> +void

> +weston_debug_scope_destroy(struct weston_debug_scope *scope);

> +

> +bool

> +weston_debug_scope_is_enabled(struct weston_debug_scope *scope);

> +

> +void

> +weston_debug_scope_write(struct weston_debug_scope *scope,

> +			 const char *data, size_t len);

> +

> +void

> +weston_debug_scope_vprintf(struct weston_debug_scope *scope,

> +			   const char *fmt, va_list ap);

> +

> +void

> +weston_debug_scope_printf(struct weston_debug_scope *scope,

> +			  const char *fmt, ...)

> +			  __attribute__ ((format (printf, 2, 3)));

> +

> +void

> +weston_debug_stream_write(struct weston_debug_stream *stream,

> +			  const char *data, size_t len);

> +

> +void

> +weston_debug_stream_vprintf(struct weston_debug_stream *stream,

> +			    const char *fmt, va_list ap);

> +

> +void

> +weston_debug_stream_printf(struct weston_debug_stream *stream,

> +			   const char *fmt, ...)

> +			   __attribute__ ((format (printf, 2, 3)));

> +

> +void

> +weston_debug_stream_complete(struct weston_debug_stream *stream);

> +

> +char *

> +weston_debug_scope_timestamp(struct weston_debug_scope *scope,

> +			     char *buf, size_t len);

> +

> +#ifdef  __cplusplus

> +}

> +#endif

> +

> +#endif /* WESTON_DEBUG_H */

> --

> 2.17.1

> 

> _______________________________________________

> wayland-devel mailing list

> wayland-devel@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
On Fri, 20 Jul 2018 20:03:24 +0100
Daniel Stone <daniels@collabora.com> wrote:

> From: Pekka Paalanen <pq@iki.fi>
> 
> weston_debug is both a libweston API for relaying debugging messages,
> and the compositor-debug wayland protocol implementation for accessing those
> debug messages from a Wayland client.
> 
> weston_debug_compositor_{create,destroy}() are private API, hence not
> exported.
> 
> Signed-off-by: Pekka Paalanen <pq@iki.fi>
> 
> append the debug scope name along with the timestamp in
> weston_debug_scope_timestamp API
> 
> Signed-off-by: Maniraj Devadoss <Maniraj.Devadoss@in.bosch.com>
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> 
> Add explicit advertisement of debug scope names.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
>  Makefile.am              |   2 +
>  libweston/compositor.c   |   6 +
>  libweston/compositor.h   |   9 +
>  libweston/weston-debug.c | 723 +++++++++++++++++++++++++++++++++++++++
>  libweston/weston-debug.h | 107 ++++++
>  5 files changed, 847 insertions(+)
>  create mode 100644 libweston/weston-debug.c
>  create mode 100644 libweston/weston-debug.h
> 

...

> diff --git a/libweston/weston-debug.c b/libweston/weston-debug.c
> new file mode 100644
> index 000000000..039247f14
> --- /dev/null
> +++ b/libweston/weston-debug.c
> @@ -0,0 +1,723 @@
> +/*
> + * Copyright © 2017 Pekka Paalanen <pq@iki.fi>
> + *
> + * 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 "config.h"
> +
> +#include "weston-debug.h"
> +#include "helpers.h"
> +#include "compositor.h"
> +
> +#include "weston-debug-server-protocol.h"
> +
> +#include <unistd.h>
> +#include <stdarg.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <sys/time.h>
> +
> +/** Main weston-debug context
> + *
> + * One per weston_compositor.
> + *
> + * \internal
> + */
> +struct weston_debug_compositor {
> +	struct weston_compositor *compositor;
> +	struct wl_listener compositor_destroy_listener;
> +	struct wl_global *global;
> +	struct wl_list scope_list; /**< weston_debug_scope::compositor_link */
> +
> +	struct weston_debug_scope *list;

Hi Daniel,

'list' member is now unused, replaced by the explicit advertisement
events.


> +};
> +
> +/** weston-debug message scope
> + *
> + * This is used for scoping debugging messages. Clients can subscribe to
> + * only the scopes they are interested in. A scope is identified by its name
> + * (also referred to as debug stream name).
> + */
> +struct weston_debug_scope {
> +	char *name;
> +	char *desc;

'desc' is never used anymore. This was meant to give the human looking
at the list of debug scopes a description of what they are, but looks
like the protocol does not carry it.

Do we not need it?


> +	weston_debug_scope_cb begin_cb;
> +	void *user_data;
> +	struct wl_list stream_list; /**< weston_debug_stream::scope_link */
> +	struct wl_list compositor_link;
> +};
> +
> +/** A debug stream created by a client
> + *
> + * A client provides a file descriptor for the server to write debug
> + * messages into. A weston_debug_stream is associated to one
> + * weston_debug_scope via the scope name, and the scope provides the messages.
> + * There can be several streams for the same scope, all streams getting the
> + * same messages.
> + */
> +struct weston_debug_stream {
> +	int fd;				/**< client provided fd */
> +	struct wl_resource *resource;	/**< weston_debug_stream_v1 object */
> +	struct wl_list scope_link;
> +};

...

> +static void
> +bind_weston_debug(struct wl_client *client,
> +		   void *data, uint32_t version, uint32_t id)
> +{
> +	struct weston_debug_compositor *wdc = data;
> +	struct weston_debug_scope *scope;
> +	struct wl_resource *resource;
> +
> +	resource = wl_resource_create(client,
> +				      &weston_debug_v1_interface,
> +				      version, id);
> +	if (!resource) {
> +		wl_client_post_no_memory(client);
> +		return;
> +	}
> +	wl_resource_set_implementation(resource, &weston_debug_impl,
> +				       wdc, NULL);
> +
> +       wl_list_for_each(scope, &wdc->scope_list, compositor_link)
> +	       weston_debug_v1_send_available(resource, scope->name);

Whitespace issues in indentation.


Otherwise looks good to me.


Thanks,
pq
On Fri, 20 Jul 2018 20:03:24 +0100
Daniel Stone <daniels@collabora.com> wrote:

> From: Pekka Paalanen <pq@iki.fi>
> 
> weston_debug is both a libweston API for relaying debugging messages,
> and the compositor-debug wayland protocol implementation for accessing those
> debug messages from a Wayland client.
> 
> weston_debug_compositor_{create,destroy}() are private API, hence not
> exported.
> 
> Signed-off-by: Pekka Paalanen <pq@iki.fi>
> 
> append the debug scope name along with the timestamp in
> weston_debug_scope_timestamp API
> 
> Signed-off-by: Maniraj Devadoss <Maniraj.Devadoss@in.bosch.com>
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> 
> Add explicit advertisement of debug scope names.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
>  Makefile.am              |   2 +
>  libweston/compositor.c   |   6 +
>  libweston/compositor.h   |   9 +
>  libweston/weston-debug.c | 723 +++++++++++++++++++++++++++++++++++++++
>  libweston/weston-debug.h | 107 ++++++
>  5 files changed, 847 insertions(+)
>  create mode 100644 libweston/weston-debug.c
>  create mode 100644 libweston/weston-debug.h

...

> +/** Format current time as a string
> + * and append the debug scope name to it
> + *
> + * \param scope[in] debug scope.
> + * \param buf[out] Buffer to store the string.
> + * \param len Available size in the buffer in bytes.
> + * \return \c buf
> + *
> + * Reads the current local wall-clock time and formats it into a string.
> + * and append the debug scope name to it.
> + * The string is nul-terminated, even if truncated.
> + */
> +WL_EXPORT char *
> +weston_debug_scope_timestamp(struct weston_debug_scope *scope,
> +			     char *buf, size_t len)
> +{
> +	struct timeval tv;
> +	struct tm *bdt;
> +	char string[128];
> +	size_t ret = 0;
> +
> +	gettimeofday(&tv, NULL);
> +
> +	bdt = localtime(&tv.tv_sec);
> +	if (bdt)
> +		ret = strftime(string, sizeof string,
> +			       "%Y-%m-%d %H:%M:%S", bdt);
> +
> +	if (ret > 0)
> +		snprintf(buf, len, "[%s.%03ld][%s]", string,
> +			 tv.tv_usec / 1000, scope->name);
> +	else
> +		snprintf(buf, len, "[?][%s]", scope->name);
> +
> +	return buf;
> +}

Hi,

realized something when looking at the "log" debug scope patch:
weston_debug_scope_timestamp() should probably be resilient against
scope == NULL, all the other functions already are.

weston_log gets initialized early, but the log debug scope gets
initialized after the compositor. If something logs something between
those two...


Thanks,
pq
On Fri, 20 Jul 2018 20:03:24 +0100
Daniel Stone <daniels@collabora.com> wrote:

> From: Pekka Paalanen <pq@iki.fi>
> 
> weston_debug is both a libweston API for relaying debugging messages,
> and the compositor-debug wayland protocol implementation for accessing those
> debug messages from a Wayland client.
> 
> weston_debug_compositor_{create,destroy}() are private API, hence not
> exported.
> 
> Signed-off-by: Pekka Paalanen <pq@iki.fi>
> 
> append the debug scope name along with the timestamp in
> weston_debug_scope_timestamp API
> 
> Signed-off-by: Maniraj Devadoss <Maniraj.Devadoss@in.bosch.com>
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> 
> Add explicit advertisement of debug scope names.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
>  Makefile.am              |   2 +
>  libweston/compositor.c   |   6 +
>  libweston/compositor.h   |   9 +
>  libweston/weston-debug.c | 723 +++++++++++++++++++++++++++++++++++++++
>  libweston/weston-debug.h | 107 ++++++
>  5 files changed, 847 insertions(+)
>  create mode 100644 libweston/weston-debug.c
>  create mode 100644 libweston/weston-debug.h
> 

Hi,

yet another thing I noticed.

> +/** Write data into a specific debug stream
> + *
> + * \param stream The debug stream to write into.
> + * \param data[in] Pointer to the data to write.
> + * \param len Number of bytes to write.
> + *
> + * Writes the given data (binary verbatim) into the debug stream.
> + * If \c len is zero or negative, the write is silently dropped.
> + *
> + * Writing is continued until all data has been written or
> + * a write fails. If the write fails due to a signal, it is re-tried.
> + * Otherwise on failure, the stream is closed and
> + * \c weston_debug_stream_v1.failure event is sent to the client.
> + *
> + * \memberof weston_debug_stream
> + */
> +WL_EXPORT void
> +weston_debug_stream_write(struct weston_debug_stream *stream,
> +			  const char *data, size_t len)
> +{
> +	ssize_t len_ = len;
> +	ssize_t ret;
> +	int e;
> +
> +	if (stream->fd == -1)
> +		return;
> +
> +	while (len_ > 0) {
> +		ret = write(stream->fd, data, len_);
> +		e = errno;
> +		if (ret < 0) {
> +			if (e == EINTR)
> +				continue;
> +
> +			stream_close_on_failure(stream,
> +					"Error writing %zd bytes: %s (%d)",
> +					len_, strerror(e), e);
> +			break;
> +		}
> +
> +		len_ -= ret;
> +		data += ret;

If write() starts returning zero, we might be in for a very long loop.

> +	}
> +}


Thanks,
pq
Hi Pekka,

On Mon, 6 Aug 2018 at 12:16, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Fri, 20 Jul 2018 20:03:24 +0100 Daniel Stone <daniels@collabora.com> wrote:
> > +/** Format current time as a string
> > + * and append the debug scope name to it
> > + *
> > + * \param scope[in] debug scope.
> > + * \param buf[out] Buffer to store the string.
> > + * \param len Available size in the buffer in bytes.
> > + * \return \c buf
> > + *
> > + * Reads the current local wall-clock time and formats it into a string.
> > + * and append the debug scope name to it.
> > + * The string is nul-terminated, even if truncated.
> > + */
> > +WL_EXPORT char *
> > +weston_debug_scope_timestamp(struct weston_debug_scope *scope,
> > +                          char *buf, size_t len)
> > +{
> > +     struct timeval tv;
> > +     struct tm *bdt;
> > +     char string[128];
> > +     size_t ret = 0;
> > +
> > +     gettimeofday(&tv, NULL);
> > +
> > +     bdt = localtime(&tv.tv_sec);
> > +     if (bdt)
> > +             ret = strftime(string, sizeof string,
> > +                            "%Y-%m-%d %H:%M:%S", bdt);
> > +
> > +     if (ret > 0)
> > +             snprintf(buf, len, "[%s.%03ld][%s]", string,
> > +                      tv.tv_usec / 1000, scope->name);
> > +     else
> > +             snprintf(buf, len, "[?][%s]", scope->name);
> > +
> > +     return buf;
> > +}
>
> realized something when looking at the "log" debug scope patch:
> weston_debug_scope_timestamp() should probably be resilient against
> scope == NULL, all the other functions already are.
>
> weston_log gets initialized early, but the log debug scope gets
> initialized after the compositor. If something logs something between
> those two...

The users introduced in this patchset already check if the scope is
enabled, which bails out if the scope is NULL. Given that, and that I
can't see a sensible behaviour if the scope is NULL, I'm inclined to
add an assert(scope) at the top of the function as well as
documentation. Does that seem reasonable? Would assert(scope) be
better or assert(weston_debug_scope_is_enabled(scope))?

Cheers,
Daniel
On Thu, 23 Aug 2018 17:53:07 +0100
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi Pekka,
> 
> On Mon, 6 Aug 2018 at 12:16, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Fri, 20 Jul 2018 20:03:24 +0100 Daniel Stone <daniels@collabora.com> wrote:  
> > > +/** Format current time as a string
> > > + * and append the debug scope name to it
> > > + *
> > > + * \param scope[in] debug scope.
> > > + * \param buf[out] Buffer to store the string.
> > > + * \param len Available size in the buffer in bytes.
> > > + * \return \c buf
> > > + *
> > > + * Reads the current local wall-clock time and formats it into a string.
> > > + * and append the debug scope name to it.
> > > + * The string is nul-terminated, even if truncated.
> > > + */
> > > +WL_EXPORT char *
> > > +weston_debug_scope_timestamp(struct weston_debug_scope *scope,
> > > +                          char *buf, size_t len)
> > > +{
> > > +     struct timeval tv;
> > > +     struct tm *bdt;
> > > +     char string[128];
> > > +     size_t ret = 0;
> > > +
> > > +     gettimeofday(&tv, NULL);
> > > +
> > > +     bdt = localtime(&tv.tv_sec);
> > > +     if (bdt)
> > > +             ret = strftime(string, sizeof string,
> > > +                            "%Y-%m-%d %H:%M:%S", bdt);
> > > +
> > > +     if (ret > 0)
> > > +             snprintf(buf, len, "[%s.%03ld][%s]", string,
> > > +                      tv.tv_usec / 1000, scope->name);
> > > +     else
> > > +             snprintf(buf, len, "[?][%s]", scope->name);
> > > +
> > > +     return buf;
> > > +}  
> >
> > realized something when looking at the "log" debug scope patch:
> > weston_debug_scope_timestamp() should probably be resilient against
> > scope == NULL, all the other functions already are.
> >
> > weston_log gets initialized early, but the log debug scope gets
> > initialized after the compositor. If something logs something between
> > those two...  
> 
> The users introduced in this patchset already check if the scope is
> enabled, which bails out if the scope is NULL. Given that, and that I
> can't see a sensible behaviour if the scope is NULL, I'm inclined to
> add an assert(scope) at the top of the function as well as
> documentation. Does that seem reasonable? Would assert(scope) be
> better or assert(weston_debug_scope_is_enabled(scope))?

Hi Daniel,

after "compositor: offer logs via weston-debug",
main.c:custom_handler() will call weston_debug_scope_timestamp()
unchecked.

Relying on libwayland not logging anything until the debug scope has
been set up seems a bit too fragile to me.

We could forbid calling weston_debug_scope_timestamp() with NULL scope,
but I think it would be easier if it just accepted NULL by doing
whatever non-crashy. That would be easier for developers. It could
simply replace the scope name with "unknown" for instance. The
resulting string will probably never be used, so it might just make an
empty string too.



On another matter, I've been wondering if storing the debug scopes in
weston_compositor is a good thing. We have the logger initialized
earlier, and quite a lot might happen before weston_compositor is
created. Also the idea of a circular buffer for after-the-fact
reporting might benefit from having debug scopes set up early, and the
command line argument to enable debug scopes from launch.


Thanks,
pq
Hi Pekka,

On Mon, 6 Aug 2018 at 14:05, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Fri, 20 Jul 2018 20:03:24 +0100 Daniel Stone <daniels@collabora.com> wrote:
> > +     while (len_ > 0) {
> > +             ret = write(stream->fd, data, len_);
> > +             e = errno;
> > +             if (ret < 0) {
> > +                     if (e == EINTR)
> > +                             continue;
> > +
> > +                     stream_close_on_failure(stream,
> > +                                     "Error writing %zd bytes: %s (%d)",
> > +                                     len_, strerror(e), e);
> > +                     break;
> > +             }
> > +
> > +             len_ -= ret;
> > +             data += ret;
>
> If write() starts returning zero, we might be in for a very long loop.

I don't think there's much we can do about that to be honest. I guess
we could post an error to the owning client and disconnect it? Else
we're just throwing away data, potentially corrupting the debug
stream, which seems suboptimal.

Currently we just block, and document that your debug client _must_
keep up with the incoming stream.

Cheers,
Daniel
Hi Pekka,

On Fri, 24 Aug 2018 at 09:12, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Thu, 23 Aug 2018 17:53:07 +0100 Daniel Stone <daniel@fooishbar.org> wrote:
> > The users introduced in this patchset already check if the scope is
> > enabled, which bails out if the scope is NULL. Given that, and that I
> > can't see a sensible behaviour if the scope is NULL, I'm inclined to
> > add an assert(scope) at the top of the function as well as
> > documentation. Does that seem reasonable? Would assert(scope) be
> > better or assert(weston_debug_scope_is_enabled(scope))?
>
> after "compositor: offer logs via weston-debug",
> main.c:custom_handler() will call weston_debug_scope_timestamp()
> unchecked.
>
> Relying on libwayland not logging anything until the debug scope has
> been set up seems a bit too fragile to me.
>
> We could forbid calling weston_debug_scope_timestamp() with NULL scope,
> but I think it would be easier if it just accepted NULL by doing
> whatever non-crashy. That would be easier for developers. It could
> simply replace the scope name with "unknown" for instance. The
> resulting string will probably never be used, so it might just make an
> empty string too.

OK, good point. Generally I hate doing nonsensical/non-helpful things
in unexpected conditions we shouldn't be called, but I don't see an
easy way out of this one for now.

> On another matter, I've been wondering if storing the debug scopes in
> weston_compositor is a good thing. We have the logger initialized
> earlier, and quite a lot might happen before weston_compositor is
> created. Also the idea of a circular buffer for after-the-fact
> reporting might benefit from having debug scopes set up early, and the
> command line argument to enable debug scopes from launch.

Yeah, I think we can definitely rework this to push the scope of the
debug stuff up. As noted in
https://gitlab.freedesktop.org/wayland/weston/issues/133 I think it
would be really good to have all this information as early as
possible, and maybe even outliving the compositor. From a libweston
API point of view, having the same API for general logging as we do
for debug makes a huge amount of sense.

Given how invasive that would be, my suggestion would be that we track
that as a follow-up issue. Once we've got all the functionality in
place we can see for sure what the best API/structure would be.

Cheers,
Daniel
On Mon, 27 Aug 2018 16:15:23 +0100
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi Pekka,
> 
> On Mon, 6 Aug 2018 at 14:05, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Fri, 20 Jul 2018 20:03:24 +0100 Daniel Stone <daniels@collabora.com> wrote:  
> > > +     while (len_ > 0) {
> > > +             ret = write(stream->fd, data, len_);
> > > +             e = errno;
> > > +             if (ret < 0) {
> > > +                     if (e == EINTR)
> > > +                             continue;
> > > +
> > > +                     stream_close_on_failure(stream,
> > > +                                     "Error writing %zd bytes: %s (%d)",
> > > +                                     len_, strerror(e), e);
> > > +                     break;
> > > +             }
> > > +
> > > +             len_ -= ret;
> > > +             data += ret;  
> >
> > If write() starts returning zero, we might be in for a very long loop.  
> 
> I don't think there's much we can do about that to be honest. I guess
> we could post an error to the owning client and disconnect it? Else
> we're just throwing away data, potentially corrupting the debug
> stream, which seems suboptimal.
> 
> Currently we just block, and document that your debug client _must_
> keep up with the incoming stream.

Hi Daniel,

yes, so if write() returns zero, something is really wrong, since the
blocking behaviour is supposed to prevent that, right?

I think disconnecting the client is better than spinning forever or
losing bits of data, but is there a case where we should spin at least
a little? Implementing a timeout seems overkill and bad for other
reasons, so if occasional zero returns are normal, then the code is ok
as is.

There is also 'failure' event we could use if disconnecting is too much.

I'm also ok with not caring about this issue until someone hits it.


Thanks,
pq
On Mon, 27 Aug 2018 16:20:41 +0100
Daniel Stone <daniel@fooishbar.org> wrote:

> Hi Pekka,
> 
> On Fri, 24 Aug 2018 at 09:12, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On another matter, I've been wondering if storing the debug scopes in
> > weston_compositor is a good thing. We have the logger initialized
> > earlier, and quite a lot might happen before weston_compositor is
> > created. Also the idea of a circular buffer for after-the-fact
> > reporting might benefit from having debug scopes set up early, and the
> > command line argument to enable debug scopes from launch.  
> 
> Yeah, I think we can definitely rework this to push the scope of the
> debug stuff up. As noted in
> https://gitlab.freedesktop.org/wayland/weston/issues/133 I think it
> would be really good to have all this information as early as
> possible, and maybe even outliving the compositor. From a libweston
> API point of view, having the same API for general logging as we do
> for debug makes a huge amount of sense.
> 
> Given how invasive that would be, my suggestion would be that we track
> that as a follow-up issue. Once we've got all the functionality in
> place we can see for sure what the best API/structure would be.

Agreed.


Thanks,
pq