[PATCH weston v4 2/9] libweston: add weston_debug API and implementation

[Daniel Stone]

Hi,

On 12 October 2017 at 13:13, Emre Ucan <eucan at de.adit-jv.com> wrote:
> +static void
> +list_scope_begin(struct weston_debug_stream *stream, void *user_data)
> +{
> +       struct weston_debug_compositor *wdc = user_data;
> +       struct weston_debug_scope *scope;
> +
> +       weston_debug_stream_printf(stream, "Supported debug streams:\n");
> +
> +       wl_list_for_each(scope, &wdc->scope_list, compositor_link) {
> +               weston_debug_stream_printf(stream, "\n * Name: %s\n", scope->name);
> +               weston_debug_stream_write(stream,
> +                                         scope->desc, strlen(scope->desc));
> +       }

This is similarly unparseable to clients. Please can we eliminate the
header message, and just emit 'NAME\tDESCRIPTION\n' for each scope?
That's still entirely human-readable, but avoids the whole procfs
mess, where the idea was to create an interface equally parseable by
the human eye and tools, inserted some unnecessary and inconsistent
pretty-printing, and failed at both.

> +static struct weston_debug_scope *
> +list_scope_init(struct weston_debug_compositor *wdc)
> +{
> +       struct weston_debug_scope *scope;
> +
> +       static const char desc[] =
> +               "Prints the list of available debug stream names.\n";

... and remove the '\n' here. Allowing arbitrary line breaks in
descriptions, rather than enforcing only one at the end, means that
tools have to search for a magic '\n * Name: 'prefix. It's not pretty.

> +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");

This is what I mean about the confusion. Part of the interface tries
for maximal pretty-printing, and another prints an ugly (if at least
very concise) 'MEMFAIL' message which is clearly more aimed at tools
than humans.

> +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);
> +       }

And we're back to maximal pretty-printing, even with a full stop. /o\

(I also don't think it would be the worst idea to have a kind of
debug-scope registry. Just a stream of name + human-readable
description + machine-readable flags would be fine.

> +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);

cf. MEMFAIL

> +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");

Ditto.

> +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";

And here.

Cheers,
Daniel