[PATCH xorg-gtest 2/2] xserver: install default X error handler
Peter Hutterer
peter.hutterer at who-t.net
Tue Nov 6 22:45:08 PST 2012
On Wed, Nov 07, 2012 at 12:56:53AM -0500, Jasper St. Pierre wrote:
> 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"
first, bad nomenclature on my side here - "clients" == tests written against
xorg-gtest.
The basic idea however is that xorg-gtest is a framework that makes starting
X servers and associates easy and manageable. anything test-specific goes
elsewhere. Now, whether the error trapping code is provided here or the
tests implement their own is up for discussion, but the current code we have
in XIT is unsuitable. Unless we trap once per test (in which case trapping
again in the test will cause an error) we cannot use it as-is.
This patch however integrates with the current trapping code: for explicit
trapping we can use SetErrorTrap/ReleaseErrorTrap, outside of that it falls
back to the default error handler provided here. Which is really just a "do
not exit(1) and break all my tests" (all tests are in the same process,
so if the default handler calls exit(1) we don't just terminate the current
test but all future ones, and we don't generate useful logs)
Cheers,
Peter
>
>
> On Wed, Nov 7, 2012 at 12:55 AM, Peter Hutterer <peter.hutterer at who-t.net>wrote:
>
> > 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.
> >
> > 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
> >
> > > On Tue, Nov 6, 2012 at 10: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");
> > > > + }
> > > > +
> > > > + 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
More information about the xorg-devel
mailing list