Right, but it seems to have one error handling mechanism in xorg-gtest, and another in xorg-integration-tests, especially with your commit message of "other clients will have to install their own error handlers" <div class="gmail_extra"> <div class="gmail_quote">On Wed, Nov 7, 2012 at 12:55 AM, Peter Hutterer <<a href="mailto:peter.hutterer@who-t.net" target="_blank">peter.hutterer@who-t.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Wed, Nov 07, 2012 at 12:36:16AM -0500, Jasper St. Pierre wrote: > I wonder if we should move the error trap system into xorg-gtest. </div>yes, it would be useful have a unified error system for (both?) xorg-gtest users. this is largely orthogonal to this commit though. the error trap system is useful to trap specific errors that are expected, but not general errors that may occur accidentally - which is what this patch addresses. Cheers, Peter <div class="HOEnZb"><div class="h5"> > On Tue, Nov 6, 2012 at 10:57 PM, Peter Hutterer <<a href="mailto:peter.hutterer@who-t.net">peter.hutterer@who-t.net</a>>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 <<a href="mailto:peter.hutterer@who-t.net">peter.hutterer@who-t.net</a>> > > --- > > 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"); > > + } > > + > > + Process::Start(server_binary, args); > > /* noreturn */ > > > > } > > @@ -494,6 +545,7 @@ void xorg::testing::XServer::Start(const std::string > > &program) { > > signal(SIGUSR1 ,SIG_IGN); > > > > RegisterXIOErrorHandler(); > > + RegisterXErrorHandler(); > > } > > > > bool xorg::testing::XServer::Terminate(unsigned int timeout) { > > diff --git a/test/xserver-test.cpp b/test/xserver-test.cpp > > index cdf0bd9..32792e6 100644 > > --- a/test/xserver-test.cpp > > +++ b/test/xserver-test.cpp > > @@ -6,6 +6,7 @@ > > #include <stdexcept> > > > > #include <xorg/gtest/xorg-gtest.h> > > +#include <gtest/gtest-spi.h> > > #include <X11/extensions/XInput2.h> > > > > using namespace xorg::testing; > > @@ -213,6 +214,78 @@ TEST(XServer, IOErrorException) > > }, XIOError); > > } > > > > +TEST(XServer, ErrorHandler) > > +{ > > + XORG_TESTCASE("Start a server, trigger a BadColor error and expect a " > > + "failure from the default error handler\n"); > > + > > + pid_t pid = fork(); > > + > > + if (pid == 0) { > > + EXPECT_NONFATAL_FAILURE({ > > + XServer server; > > + server.SetOption("-logfile", LOGFILE_DIR > > "/xorg-error-handler-test.log"); > > + server.SetOption("-config", DUMMY_CONF_PATH); > > + server.SetOption("-noreset", ""); > > + server.Start(); > > + ASSERT_EQ(server.GetState(), Process::RUNNING); > > + ::Display *dpy = XOpenDisplay(server.GetDisplayString().c_str()); > > + ASSERT_TRUE(dpy != NULL); > > + XColor color; > > + XQueryColor(dpy, 0, &color); > > + XSync(dpy, False); > > + }, "BadColor"); > > + exit(0); > > + } > > + > > + /* if the Xlib default error handler triggers, child calls exit(1) */ > > + int status; > > + ASSERT_EQ(waitpid(pid, &status, 0), pid); > > + ASSERT_TRUE(WIFEXITED(status)); > > + ASSERT_EQ(WEXITSTATUS(status), 0); > > +} > > + > > +static bool error_handler_triggered = false; > > + > > +static int _test_error_handler(Display *dpy, XErrorEvent *error) { > > + error_handler_triggered = true; > > + if (error->error_code != BadColor) > > + ADD_FAILURE() << "Error handler triggered with wrong error code\n"; > > + return 0; > > +} > > + > > +TEST(XServer, NondefaultErrorHandler) > > +{ > > + XORG_TESTCASE("Set a custom error handler, start a server, trigger a " > > + "BadColor error and expect the custom error handler to be > > " > > + "triggered\n"); > > + > > + pid_t pid = fork(); > > + > > + if (pid == 0) { > > + XSetErrorHandler(_test_error_handler); > > + > > + XServer server; > > + server.SetOption("-logfile", LOGFILE_DIR > > "/xorg-error-handler-test.log"); > > + server.SetOption("-config", DUMMY_CONF_PATH); > > + server.SetOption("-noreset", ""); > > + server.Start(); > > + ASSERT_EQ(server.GetState(), Process::RUNNING); > > + ::Display *dpy = XOpenDisplay(server.GetDisplayString().c_str()); > > + ASSERT_TRUE(dpy != NULL); > > + XColor color; > > + XQueryColor(dpy, 0, &color); > > + XSync(dpy, False); > > + exit(error_handler_triggered ? 0 : 1); > > + } > > + > > + /* if the Xlib default error handler triggers, child calls exit(1) */ > > + int status; > > + ASSERT_EQ(waitpid(pid, &status, 0), pid); > > + ASSERT_TRUE(WIFEXITED(status)); > > + ASSERT_EQ(WEXITSTATUS(status), 0); > > +} > > + > > TEST(XServer, KeepAlive) > > { > > XORG_TESTCASE("If XORG_GTEST_XSERVER_KEEPALIVE is set,\n" > > -- > > 1.7.11.7 > > > > _______________________________________________ > > <a href="mailto:xorg-devel@lists.x.org">xorg-devel@lists.x.org</a>: X.Org development > > Archives: <a href="http://lists.x.org/archives/xorg-devel" target="_blank">http://lists.x.org/archives/xorg-devel</a> > > Info: <a href="http://lists.x.org/mailman/listinfo/xorg-devel" target="_blank">http://lists.x.org/mailman/listinfo/xorg-devel</a> > > > > > > -- > Jasper </div></div></blockquote></div> -- Jasper </div>