[PATCH xorg-gtest 2/2] xserver: install default X error handler

[Peter Hutterer]

On Thu, Nov 08, 2012 at 05:46:12PM -0800, Chase Douglas wrote:
> On Tue, Nov 6, 2012 at 7:57 PM, Peter Hutterer <peter.hutterer at who-t.net>wrote:
> 
> > Xlib's default error handler prints the error and calls exit(1). Tests that
> > accidentally trigger an error thus quit without cleaning up properly.
> >
> > Install a default error handler that prints the basic info and continue
> > with
> > the test. Clients that expect to trigger errors should set a custom error
> > handler.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> >  include/xorg/gtest/xorg-gtest-xserver.h | 13 ++++++
> >  src/xserver.cpp                         | 54 +++++++++++++++++++++++-
> >  test/xserver-test.cpp                   | 73
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 139 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/xorg/gtest/xorg-gtest-xserver.h
> > b/include/xorg/gtest/xorg-gtest-xserver.h
> > index 8bf7996..11fc93d 100644
> > --- a/include/xorg/gtest/xorg-gtest-xserver.h
> > +++ b/include/xorg/gtest/xorg-gtest-xserver.h
> > @@ -265,6 +265,19 @@ class XServer : public xorg::testing::Process {
> >       */
> >      static void RegisterXIOErrorHandler();
> >
> > +    /**
> > +     * Install a default XErrorHandler. That error handler will cause a
> > test
> > +     * failure if called.
> > +     *
> > +     * This function is called automatically by XServer::Start(). Usually,
> > +     * you will not need to call this function unless your test does not
> > +     * instantiate and Start() an XServer object.
> > +     *
> > +     * This function will only install a new error handler if the
> > currently
> > +     * installed XErrorHandler is not the default handler used by Xlib.
> > +     */
> > +    static void RegisterXErrorHandler();
> > +
> >    private:
> >      struct Private;
> >      std::auto_ptr<Private> d_;
> > diff --git a/src/xserver.cpp b/src/xserver.cpp
> > index ad018a1..4faa8e9 100644
> > --- a/src/xserver.cpp
> > +++ b/src/xserver.cpp
> > @@ -394,6 +394,40 @@ const std::string&
> > xorg::testing::XServer::GetVersion(void) {
> >    return d_->version;
> >  }
> >
> > +static int _x_error_handler(Display *dpy, XErrorEvent *err)
> > +{
> > +  std::stringstream error;
> > +  switch(err->error_code) {
> > +    case BadRequest: error << "BadRequest"; break;
> > +    case BadValue: error << "BadValue"; break;
> > +    case BadWindow: error << "BadWindow"; break;
> > +    case BadPixmap: error << "BadPixmap"; break;
> > +    case BadAtom: error << "BadAtom"; break;
> > +    case BadCursor: error << "BadCursor"; break;
> > +    case BadFont: error << "BadFont"; break;
> > +    case BadMatch: error << "BadMatch"; break;
> > +    case BadDrawable: error << "BadDrawable"; break;
> > +    case BadAccess: error << "BadAccess"; break;
> > +    case BadAlloc: error << "BadAlloc"; break;
> > +    case BadColor: error << "BadColor"; break;
> > +    case BadGC: error << "BadGC"; break;
> > +    case BadIDChoice: error << "BadIDChoice"; break;
> > +    case BadName: error << "BadName"; break;
> > +    case BadLength: error << "BadLength"; break;
> > +    case BadImplementation: error << "BadImplementation"; break;
> > +    default:
> > +      error << err->error_code;
> > +      break;
> > +  }
> > +
> > +  ADD_FAILURE() << "XError received: " << error.str() << ", request " <<
> > +    (int)err->request_code << "(" << (int)err->minor_code << "), detail: "
> > +    << err->resourceid << "\nThis error handler is likely to be triggered
> > "
> > +    "more than once.\nCheck the first error for the real error";
> > +  return 0;
> > +}
> > +
> > +
> >  static int _x_io_error_handler(Display *dpy) _X_NORETURN;
> >  static int _x_io_error_handler(Display *dpy)
> >  {
> > @@ -409,6 +443,15 @@ void xorg::testing::XServer::RegisterXIOErrorHandler()
> >      XSetIOErrorHandler(old_handler);
> >  }
> >
> > +void xorg::testing::XServer::RegisterXErrorHandler()
> > +{
> > +  XErrorHandler old_handler;
> > +  old_handler = XSetErrorHandler(_x_error_handler);
> > +
> > +  if (old_handler != _XDefaultError)
> > +    XSetErrorHandler(old_handler);
> > +}
> > +
> >  void xorg::testing::XServer::Start(const std::string &program) {
> >    TestStartup();
> >
> > @@ -464,7 +507,15 @@ void xorg::testing::XServer::Start(const std::string
> > &program) {
> >          args.push_back(it->second);
> >      }
> >
> > -    Process::Start(program.empty() ? d_->path_to_server : program, args);
> > +    std::string server_binary = program.empty() ? d_->path_to_server :
> > program;
> > +
> > +    if (getenv("XORG_GTEST_XSERVER_USE_VALGRIND")) {
> > +      args.insert(args.begin(), server_binary);
> > +      server_binary = "valgrind";
> > +      args.insert(args.begin(), "--leak-check=full");
> > +    }
> >
> 
> While this looks totally cool, it probably belongs in a separate commit :).

oh, that's where that hunk ended up :) I was wondering which branch it was
on, and could not find the git commit referring to it. got committed by
accident.

> Without this hunk:
> 
> Reviewed-by: Chase Douglas <chase.douglas at ubuntu.com>
> 
> You can also add my Reviewed-by tag to a separate commit for just this
> hunk. You might want to consider allowing for some standard options like
> --show-reachable

yeah, it's not complete yet, that's why I hadn't intended to send this one
out just yet :)

Thanks for the reviews

Cheers,
   Peter