[PATCH xorg-gtest 06/16] xserver: move Terminate and Kill handling here

[Chase Douglas]

On 07/03/2012 10:42 PM, Peter Hutterer wrote:
> On Tue, Jul 03, 2012 at 10:33:50AM -0700, Chase Douglas wrote:
>> On 07/02/2012 11:44 PM, Peter Hutterer wrote:
>>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>>> ---
>>>   include/xorg/gtest/xorg-gtest-xserver.h |   13 ++++++++++++
>>>   src/environment.cpp                     |   31 +++-------------------------
>>>   src/xserver.cpp                         |   34 +++++++++++++++++++++++++++++++
>>>   3 files changed, 50 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/include/xorg/gtest/xorg-gtest-xserver.h b/include/xorg/gtest/xorg-gtest-xserver.h
>>> index 52a2fd0..821b01f 100644
>>> --- a/include/xorg/gtest/xorg-gtest-xserver.h
>>> +++ b/include/xorg/gtest/xorg-gtest-xserver.h
>>> @@ -53,6 +53,19 @@ class XServer : public xorg::testing::Process {
>>>       void Start(std::string &program);
>>>
>>>       /**
>>> +     * Terminates this server process. Will signal the server to terminate
>>> +     * multiple times before giving up.
>>> +     *
>>> +     * @return false if the server did not terminate, true otherwise
>>> +     */
>>> +    bool Terminate(void);
>>> +
>>> +    /**
>>> +     * Kills the server. With a vengeance.
>>> +     */
>>> +    bool Kill(void);
>>
>> We don't need to recreate these functions. We've already inherited
>> them from xorg::testing::Process. Those implementations should work
>> automatically if we set up the XServer class properly.
>
> The implementation is different to the one in Process, this one does the
> server-specific bits like waiting for the process to shut down and then
> complaining to the log.

I read too quickly to realize the difference between the 
Environment/XServer implementation and the Process implementation, but 
I'm wondering if we should just move the extra stuff to the Process 
implementation. Then we won't need to do any redefinition or overriding.

If we do still want separate implementations, I think we still want to 
override instead of redefine. The main reason you want to override 
rather than redefine is to ensure the correct method is always called 
for a given object. If you only have a superclass handle of an object, 
the subclass method is still called:

   Subclass sub;
   Superclass& super = dynamic_cast<Superclass&>(sub);
   super.Terminate(); // This still calls Subclass.Terminate()

All you need to do to make the method override rather than redefine is 
to add the 'virtual' keyword in front of the method declaration in the 
base class (though most people also put it in front of the declaration 
in the derived class as book-keeping). You can also delete the 
documentation if you want, since the Doxygen config is set up to inherit 
the documentation :).

>>> +
>>> +    /**
>>>        * Waits until this server is ready to take connections.
>>>        */
>>>       void WaitForConnections(void);
>>> diff --git a/src/environment.cpp b/src/environment.cpp
>>> index 69972a4..b041236 100644
>>> --- a/src/environment.cpp
>>> +++ b/src/environment.cpp
>>> @@ -116,35 +116,10 @@ void xorg::testing::Environment::SetUp() {
>>>   }
>>>
>>>   void xorg::testing::Environment::TearDown() {
>>> -  if (d_->server.Terminate()) {
>>> -    for (int i = 0; i < 10; i++) {
>>> -      int status;
>>> -      int pid = waitpid(d_->server.Pid(), &status, WNOHANG);
>>> -
>>> -      if (pid == d_->server.Pid())
>>> -        return;
>>> -
>>> -      sleep(1); /* Give the dummy X server more time to shut down */
>>> -    }
>>> -  }
>>> -
>>> -  Kill();
>>> +  if (!d_->server.Terminate())
>>> +    Kill();
>>>   }
>>>
>>>   void xorg::testing::Environment::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_->server.Pid(), &status, WNOHANG);
>>> -
>>> -    if (pid == d_->server.Pid())
>>> -      return;
>>> -
>>> -      sleep(1); /* Give the dummy X server more time to shut down */
>>> -  }
>>> -
>>> -  std::cerr << "Warning: Dummy X server did not shut down\n";
>>> +  d_->server.Kill();
>>>   }
>>> diff --git a/src/xserver.cpp b/src/xserver.cpp
>>> index 1a46dbb..bd1e2f9 100644
>>> --- a/src/xserver.cpp
>>> +++ b/src/xserver.cpp
>>> @@ -298,3 +298,37 @@ void xorg::testing::XServer::Start(std::string &program) {
>>>                    "-config", d_->path_to_conf.c_str(),
>>>                    NULL);
>>>   }
>>> +
>>> +bool xorg::testing::XServer::Terminate(void) {
>>> +  if (Process::Terminate()) {
>>> +    for (int i = 0; i < 10; i++) {
>>> +      int status;
>>> +      int pid = waitpid(Pid(), &status, WNOHANG);
>>> +
>>> +      if (pid == Pid())
>>> +        return true;
>>> +
>>> +      sleep(1); /* Give the dummy X server more time to shut down */
>>> +    }
>>> +  }
>>> +  return false;
>>> +}
>>> +
>>> +bool xorg::testing::XServer::Kill(void) {
>>> +  if (!Process::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(Pid(), &status, WNOHANG);
>>> +
>>> +    if (pid == Pid())
>>> +      return true;
>>> +
>>> +      sleep(1); /* Give the dummy X server more time to shut down */
>>> +  }
>>> +
>>> +  std::cerr << "Warning: Dummy X server did not shut down\n";
>>> +  return false;
>>> +}
>>>
>>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>