[PATCH 1/3] Make default log handler print to stderr

[Pekka Paalanen]

On Sun, 9 Feb 2014 21:35:10 -0800
Kristian Høgsberg <hoegsberg at gmail.com> wrote:

> On Sat, Feb 08, 2014 at 11:44:11AM +0200, Pekka Paalanen wrote:
> > On Fri,  7 Feb 2014 22:27:00 -0800
> > Kristian Høgsberg <krh at bitplanet.net> wrote:
> > 
> > > On the client side we log fatal errors before we exit.  If a
> > > client doesn't set a log handler, it's hard to figure out what
> > > goes wrong. ---
> > >  src/wayland-util.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/wayland-util.c b/src/wayland-util.c
> > > index 4fe9c81..b099882 100644
> > > --- a/src/wayland-util.c
> > > +++ b/src/wayland-util.c
> > > @@ -23,6 +23,7 @@
> > >  
> > >  #include <stdlib.h>
> > >  #include <stdint.h>
> > > +#include <stdio.h>
> > >  #include <string.h>
> > >  #include <stdarg.h>
> > >  
> > > @@ -361,11 +362,12 @@ wl_map_for_each(struct wl_map *map,
> > > wl_iterator_func_t func, void *data) }
> > >  
> > >  static void
> > > -wl_log_noop_handler(const char *fmt, va_list arg)
> > > +wl_log_stderr_handler(const char *fmt, va_list arg)
> > >  {
> > > +	vfprintf(stderr, fmt, arg);
> > >  }
> > >  
> > > -wl_log_func_t wl_log_handler = wl_log_noop_handler;
> > > +wl_log_func_t wl_log_handler = wl_log_stderr_handler;
> > >  
> > >  void
> > >  wl_log(const char *fmt, ...)
> > > -- 
> > 
> > Hi,
> > 
> > seems you think it is fine to printf() from libwayland-client *and*
> > libwayland-server by default, so ok by me then. You probably know
> > library conventions better than I do.
> 
> I don't think our default behavior of silently ignoring fatal errors and
> relying on the application to catch errors is fine.

Hi,

printing is fine, I see many libs or programs printing warnings
already, even non-fatal.

> The background for these three patches was that I ported simple-egl to
> xdg-shell and forgot the call to set_unstable_version().  Without this,
> weston sends an error in response to all other xdg-shell requests.
> simple-egl doesn't set a log function, so this was never printed.
> On top of that simple-egl calls wl_display_dispatch_pending() without
> error checking so the fatal error didn't halt the loop and simple-egl
> eventually crashed with a protocol buffer overflow.
> 
> That's a pretty unhelpful way to deal with errors and it took me a while
> to get to the bottom of it.  Maybe we need a configurable error handler
> instead, and then make the default handler print the error and abort.

I'm not sure about the error handler. Simple-egl's fundamental problem
was not checking the return value of wl_display_dispatch*(). If it did,
and aborted on that, it would have been much easier for you to find
out, right?

Printing errors to stderr by default from libwayland also makes the
case obvious, and even without it WAYLAND_DEBUG=client would show it.

Do you think we really need yet another mechanism which one needs to
opt-out of, or it will kill the process immediately on a protocol error?

I don't think your time was wasted investigating it, you found a bug
(the crash) in the protocol buffer handling, right?

> The third patch in this series deals with the case where an application
> doesn't dispatch the default queue for a long time (I've seen games that
> have a thread that updates a progress bar/loading screen while the main
> thread compiles shaders or such).  If an error occurs in that case we still
> need to make sure we handle it. wl_display is thread safe so we can grab
> the lock and dispatch wl_display events from any thread the calls into
> dispatch.  This applies to error but it's also important for delete_id
> events, to make sure the rendering thread recycles IDs.

Yes, the idea sounds good.


Thanks,
pq