[PATCH xorg-gtest v2 1/8] Add a new class representing the X server

Chase Douglas chase.douglas at canonical.com
Wed Jul 11 11:35:47 PDT 2012


On 07/10/2012 08:28 PM, Peter Hutterer wrote:
> This class is a Process, so it's a drop-in replacement for the current
> Environment, but it provides a few methods to talk to the server being
> started.
>
> SetDisplayNumber() is called, but currently unused by the server.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> Changes to v1:
> - squashed a few minor patches in so this class is a bit more complete out
> of the box.
>
>   configure.ac                            |    2 +-
>   include/Makefile.am                     |    1 +
>   include/xorg/gtest/xorg-gtest-xserver.h |  126 ++++++++++++++++++++
>   include/xorg/gtest/xorg-gtest.h         |    1 +
>   src/Makefile.am                         |    1 +
>   src/environment.cpp                     |   22 ++--
>   src/xorg-gtest-all.cpp                  |    1 +
>   src/xserver.cpp                         |  196 +++++++++++++++++++++++++++++++
>   8 files changed, 339 insertions(+), 11 deletions(-)
>   create mode 100644 include/xorg/gtest/xorg-gtest-xserver.h
>   create mode 100644 src/xserver.cpp
>
> diff --git a/configure.ac b/configure.ac
> index ea29c50..d9b3072 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -25,7 +25,7 @@ XORG_DEFAULT_OPTIONS
>   XORG_ENABLE_INTEGRATION_TESTS([yes])
>   XORG_WITH_DOXYGEN
>
> -PKG_CHECK_MODULES(X11, x11)
> +PKG_CHECK_MODULES(X11, x11 xi)
>
>   # Check for Google Test
>   CHECK_GTEST
> diff --git a/include/Makefile.am b/include/Makefile.am
> index 9ff5f2a..4a6ce03 100644
> --- a/include/Makefile.am
> +++ b/include/Makefile.am
> @@ -34,6 +34,7 @@ nobase_include_HEADERS = \
>   	xorg/gtest/xorg-gtest-environment.h \
>   	xorg/gtest/xorg-gtest-process.h \
>   	xorg/gtest/xorg-gtest-test.h \
> +	xorg/gtest/xorg-gtest-xserver.h \
>   	xorg/gtest/evemu/xorg-gtest-device.h \
>   	xorg/gtest/xorg-gtest.h \
>   	$(compat_headers)
> diff --git a/include/xorg/gtest/xorg-gtest-xserver.h b/include/xorg/gtest/xorg-gtest-xserver.h
> new file mode 100644
> index 0000000..56dacf6
> --- /dev/null
> +++ b/include/xorg/gtest/xorg-gtest-xserver.h
> @@ -0,0 +1,126 @@
> +/*******************************************************************************
> + *
> + * X testing environment - Google Test helper class to communicate with the
> + * server
> + *
> + * Copyright (C) 2012 Red Hat, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + ******************************************************************************/
> +
> +
> +#ifndef XORG_GTEST_XSERVER_H
> +#define XORG_GTEST_XSERVER_H
> +
> +#include <gtest/gtest.h>
> +#include <xorg/gtest/xorg-gtest.h>
> +#include <X11/Xlib.h>
> +
> +namespace xorg {
> +namespace testing {
> +
> +/**
> + * @class XServer xorg-gtest-xserver.h xorg/gtest/xorg-gtest-xserver.h

Minor niggle: doxygen is smart enough to know that we're documenting a 
class without having to tell it. I like leaving out the "@class" since 
it just clutters things up.

> + *
> + * Miscellaneous interfaces to communicate with the X server.
> + */
> +class XServer : public xorg::testing::Process {
> +  public:
> +    XServer();
> +
> +    /**
> +     * Set the display number for this server. This number must be set
> +     * before the server is started to have any effect.
> +     * If unset, the default display number is used.
> +     *
> +     * @param [in] display_number The display number the server runs on
> +     */
> +    void SetDisplayNumber(unsigned int display_number);
> +
> +    /**
> +     * Get the display number from this server. If the server was not
> +     * started yet, this function returns the display number the server will
> +     * be started on.
> +     *
> +     * @return The numeric display number this server runs on
> +     */
> +    unsigned int GetDisplayNumber(void);
> +
> +    /**
> +     * Get the display string that may be used for XOpenDisplay to this
> +     * server. This string is effectively :display_number.
> +     *
> +     * @return The display string used for XOpenDisplay() to this server.
> +     */
> +    const std::string& GetDisplayString(void);
> +
> +    /**
> +     * Wait for a specific device to be added to the server.
> +     *
> +     * @param [in] display The X display connection
> +     * @param [in] name    The name of the device to wait for
> +     * @param [in] timeout The timeout in milliseconds
> +     *
> +     * @return Whether the device was added
> +     */
> +    static bool WaitForDevice(::Display *display, const std::string &name, time_t timeout = 1000);
> +
> +    /**
> +     * Wait for an event on the X connection.
> +     *
> +     * @param [in] display The X display connection
> +     * @param [in] timeout The timeout in milliseconds
> +     *
> +     * @return Whether an event is available
> +     */
> +    static bool WaitForEvent(::Display *display, time_t timeout = 1000);
> +
> +    /**
> +     * Wait for an event of a specific type on the X connection.
> +     *
> +     * All events preceding the matching event are discarded. If no event was found
> +     * before the timeout expires, all events in the queue will have been discarded.
> +     *
> +     * @param [in] display The X display connection

Minor niggle: this ^^ param documentation is two columns left of the 
rest :).

> +     * @param [in] type      The X core protocol event type
> +     * @param [in] extension The X extension opcode of a generic event, or -1 for
> +     *                       any generic event
> +     * @param [in] evtype    The X extension event type of a generic event, or -1
> +     *                       for any event of the given extension
> +     * @param [in] timeout   The timeout in milliseconds
> +     *
> +     * @return Whether an event is available
> +     */
> +    static bool WaitForEventOfType(::Display *display, int type, int extension, int evtype, time_t timeout = 1000);
> +
> +  private:
> +    struct Private;
> +    std::auto_ptr<Private> d_;
> +
> +    /* Disable copy constructor, assignment operator */
> +    XServer(const XServer&);
> +    XServer& operator=(const XServer&);
> +
> +};
> +} // namespace testing
> +} // namespace xorg
> +
> +#endif /* XORG_GTEST_XSERVER_H */
> diff --git a/include/xorg/gtest/xorg-gtest.h b/include/xorg/gtest/xorg-gtest.h
> index 9d57e9e..6a621bd 100644
> --- a/include/xorg/gtest/xorg-gtest.h
> +++ b/include/xorg/gtest/xorg-gtest.h
> @@ -27,6 +27,7 @@
>
>   #include "xorg-gtest-environment.h"
>   #include "xorg-gtest-process.h"
> +#include "xorg-gtest-xserver.h"
>   #include "xorg-gtest-test.h"
>
>   #ifdef HAVE_EVEMU
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 0b93df0..3d7824d 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -29,6 +29,7 @@ libxorg_gtest_sources = \
>   	device.cpp \
>   	process.cpp \
>   	test.cpp \
> +	xserver.cpp \
>   	xorg-gtest-all.cpp
>
>   libxorg_gtest_main_sources = \
> diff --git a/src/environment.cpp b/src/environment.cpp
> index b02a6a2..9a524ab 100644
> --- a/src/environment.cpp
> +++ b/src/environment.cpp
> @@ -27,6 +27,7 @@
>
>   #include "xorg/gtest/xorg-gtest-environment.h"
>   #include "xorg/gtest/xorg-gtest-process.h"
> +#include "xorg/gtest/xorg-gtest-xserver.h"
>   #include "defines.h"
>
>   #include <sys/types.h>
> @@ -52,7 +53,7 @@ struct xorg::testing::Environment::Private {
>     std::string path_to_log_file;
>     std::string path_to_server;
>     int display;
> -  Process process;
> +  XServer server;
>   };
>
>   xorg::testing::Environment::Environment()
> @@ -140,7 +141,8 @@ void xorg::testing::Environment::SetUp() {
>       throw std::runtime_error(message);
>     }
>
> -  d_->process.Start(d_->path_to_server, d_->path_to_server.c_str(),
> +  d_->server.SetDisplayNumber(d_->display);
> +  d_->server.Start(d_->path_to_server, d_->path_to_server.c_str(),
>                       display_string,
>                       "-logverbose", "10",
>                       "-logfile", d_->path_to_log_file.c_str(),
> @@ -158,8 +160,8 @@ void xorg::testing::Environment::SetUp() {
>       }
>
>       int status;
> -    int pid = waitpid(d_->process.Pid(), &status, WNOHANG);
> -    if (pid == d_->process.Pid()) {
> +    int pid = waitpid(d_->server.Pid(), &status, WNOHANG);
> +    if (pid == d_->server.Pid()) {
>         std::string message;
>         message += "X server failed to start on display ";
>         message += display_string;
> @@ -183,12 +185,12 @@ void xorg::testing::Environment::SetUp() {
>   }
>
>   void xorg::testing::Environment::TearDown() {
> -  if (d_->process.Terminate()) {
> +  if (d_->server.Terminate()) {
>       for (int i = 0; i < 10; i++) {
>         int status;
> -      int pid = waitpid(d_->process.Pid(), &status, WNOHANG);
> +      int pid = waitpid(d_->server.Pid(), &status, WNOHANG);
>
> -      if (pid == d_->process.Pid())
> +      if (pid == d_->server.Pid())
>           return;
>
>         sleep(1); /* Give the dummy X server more time to shut down */
> @@ -199,15 +201,15 @@ void xorg::testing::Environment::TearDown() {
>   }
>
>   void xorg::testing::Environment::Kill() {
> -  if (!d_->process.Kill())
> +  if (!d_->server.Kill())
>       std::cerr << "Warning: Failed to kill dummy Xorg server: "
>                 << std::strerror(errno) << "\n";
>
>     for (int i = 0; i < 10; i++) {
>       int status;
> -    int pid = waitpid(d_->process.Pid(), &status, WNOHANG);
> +    int pid = waitpid(d_->server.Pid(), &status, WNOHANG);
>
> -    if (pid == d_->process.Pid())
> +    if (pid == d_->server.Pid())
>         return;
>
>         sleep(1); /* Give the dummy X server more time to shut down */
> diff --git a/src/xorg-gtest-all.cpp b/src/xorg-gtest-all.cpp
> index fab6425..85e17c0 100644
> --- a/src/xorg-gtest-all.cpp
> +++ b/src/xorg-gtest-all.cpp
> @@ -27,6 +27,7 @@
>
>   #include "src/environment.cpp"
>   #include "src/process.cpp"
> +#include "src/xserver.cpp"
>   #include "src/test.cpp"
>
>   #ifdef HAVE_EVEMU
> diff --git a/src/xserver.cpp b/src/xserver.cpp
> new file mode 100644
> index 0000000..c24cee0
> --- /dev/null
> +++ b/src/xserver.cpp
> @@ -0,0 +1,196 @@
> +/*******************************************************************************
> + *
> + * X testing environment - Google Test helper class to communicate with the
> + * server
> + *
> + * Copyright (C) 2012 Red Hat, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + ******************************************************************************/
> +
> +#include "xorg/gtest/xorg-gtest-xserver.h"
> +
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +
> +#include <algorithm>
> +#include <cerrno>
> +#include <csignal>
> +#include <cstdio>
> +#include <cstdlib>
> +#include <cstring>
> +#include <stdexcept>
> +#include <vector>
> +
> +#include <X11/extensions/XInput2.h>
> +
> +struct xorg::testing::XServer::Private {
> +  unsigned int display_number;
> +  std::string display_string;
> +};
> +
> +xorg::testing::XServer::XServer() : d_(new Private) {
> +  SetDisplayNumber(d_->display_number);

If I'm reading this correctly, d_ will be allocated with unset values. 
d_->display_number will potentially be any integer. Then, we call 
SetDisplayNumber() with the same random integer.

We should be calling SetDisplayNumber(DEFAULT_DISPLAY).

> +}
> +
> +void xorg::testing::XServer::SetDisplayNumber(unsigned int display_number) {
> +    d_->display_number = display_number;
> +
> +    std::stringstream s;
> +    s << ":" << display_number;
> +    d_->display_string = s.str();
> +}
> +
> +unsigned int xorg::testing::XServer::GetDisplayNumber(void) {
> +  return d_->display_number;
> +}
> +
> +const std::string& xorg::testing::XServer::GetDisplayString(void) {
> +  return d_->display_string;
> +}
> +
> +bool xorg::testing::XServer::WaitForEvent(::Display *display, time_t timeout)
> +{
> +    fd_set fds;
> +    FD_ZERO(&fds);
> +
> +    int display_fd = ConnectionNumber(display);
> +
> +    XSync(display, False);
> +
> +    if (XPending(display))
> +        return true;
> +    else {
> +        FD_SET(display_fd, &fds);
> +
> +        struct timeval timeval = {
> +            static_cast<time_t>(timeout / 1000),
> +            static_cast<time_t>(timeout % 1000),
> +        };
> +
> +        int ret;
> +        if (timeout)
> +            ret = select(display_fd + 1, &fds, NULL, NULL, &timeval);
> +        else
> +            ret = select(display_fd + 1, &fds, NULL, NULL, NULL);
> +
> +        if (ret < 0)
> +            throw std::runtime_error("Failed to select on X fd");
> +
> +        if (ret == 0)
> +            return false;
> +
> +        return XPending(display);
> +    }
> +}
> +
> +bool xorg::testing::XServer::WaitForEventOfType(::Display *display, int type, int extension,
> +                                                int evtype, time_t timeout)
> +{
> +    while (WaitForEvent(display)) {
> +        XEvent event;
> +        if (!XPeekEvent(display, &event))
> +            throw std::runtime_error("Failed to peek X event");
> +
> +        if (event.type != type) {
> +            if (XNextEvent(display, &event) != Success)
> +                throw std::runtime_error("Failed to remove X event");
> +            continue;
> +        }
> +
> +        if (event.type != GenericEvent || extension == -1)
> +            return true;
> +
> +        XGenericEvent *generic_event = reinterpret_cast<XGenericEvent*>(&event);
> +
> +        if (generic_event->extension != extension) {
> +            if (XNextEvent(display, &event) != Success)
> +                throw std::runtime_error("Failed to remove X event");
> +            continue;
> +        }
> +
> +        if (evtype == -1 || generic_event->evtype == evtype)
> +            return true;
> +
> +        if (XNextEvent(display, &event) != Success)
> +            throw std::runtime_error("Failed to remove X event");
> +    }
> +
> +    return false;
> +}
> +
> +bool xorg::testing::XServer::WaitForDevice(::Display *display, const std::string &name,
> +                                           time_t timeout)
> +{
> +    int opcode;
> +    int event_start;
> +    int error_start;
> +
> +    if (!XQueryExtension(display, "XInputExtension", &opcode, &event_start,
> +                         &error_start))
> +        throw std::runtime_error("Failed to query XInput extension");
> +
> +    while (WaitForEventOfType(display, GenericEvent, opcode,
> +                              XI_HierarchyChanged)) {
> +        XEvent event;
> +        if (XNextEvent(display, &event) != Success)
> +            throw std::runtime_error("Failed to get X event");
> +
> +        XGenericEventCookie *xcookie =
> +            reinterpret_cast<XGenericEventCookie*>(&event.xcookie);
> +        if (!XGetEventData(display, xcookie))
> +            throw std::runtime_error("Failed to get X event data");
> +
> +        XIHierarchyEvent *hierarchy_event =
> +            reinterpret_cast<XIHierarchyEvent*>(xcookie->data);
> +
> +        if (!(hierarchy_event->flags & XIDeviceEnabled)) {
> +            XFreeEventData(display, xcookie);
> +            continue;
> +        }
> +
> +        bool device_found = false;
> +        for (int i = 0; i < hierarchy_event->num_info; i++) {
> +            if (!(hierarchy_event->info[i].flags & XIDeviceEnabled))
> +                continue;
> +
> +            int num_devices;
> +            XIDeviceInfo *device_info =
> +                XIQueryDevice(display, hierarchy_event->info[i].deviceid,
> +                              &num_devices);
> +            if (num_devices != 1 || !device_info)
> +                throw std::runtime_error("Failed to query device");
> +
> +            if (name.compare(device_info[0].name) == 0) {
> +                device_found = true;
> +                break;
> +            }
> +        }
> +
> +        XFreeEventData(display, xcookie);
> +
> +        if (device_found)
> +            return true;
> +    }
> +
> +    return false;
> +}
>

Everything else looks good!

Reviewed-by: Chase Douglas <chase.douglas at canonical.com>


More information about the xorg-devel mailing list