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

[Pekka Paalanen]

On Mon, 23 Oct 2017 13:06:19 +0200
Daniel Stone <daniel at fooishbar.org> wrote:

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

This is a failure inside a failure path. It's not even going into the
debug stream.

You obviously have very different ideas on what the design goals of this
framework should be, and it's not a surprise the implementation does not
fit as it is not what it was written to do.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171023/ab38b1b2/attachment.sig>