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

2012-07-10 Thread Peter Hutterer 
On Mon, Jul 09, 2012 at 04:59:02PM -0700, Chase Douglas wrote:
 On 07/09/2012 04:26 PM, Peter Hutterer wrote:
 On Mon, Jul 09, 2012 at 10:19:50AM -0700, Chase Douglas wrote:
 On 07/08/2012 06:50 PM, Peter Hutterer wrote:
 On Fri, Jul 06, 2012 at 11:08:13AM -0700, Chase Douglas wrote:
 On 07/05/2012 05:50 PM, Peter Hutterer wrote:
 On Thu, Jul 05, 2012 at 02:09:13PM -0700, Chase Douglas wrote:
 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.hutte...@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.
 
 I think we should keep Process basic. For most processes failing to kill 
 it
 should be an issue in itself, it's just the server that takes too long to
 shut down and needs special handling.
 
 I thought about that too. I think the question to ask is: do we
 think most users or subclasses of Process are going to want wait for
 the process to really die. The Process::Kill and Terminate methods
 just try once to raise the signal to the child process. They return
 true if the signal was sent, but don't check what happens after
 that. Perhaps a better approach to resolving this issue would be to
 add second versions of these methods that would take a timeout value
 and return true only if the signal was successfully sent and the
 process died.
 KillAndCheck(timeout) and TerminateAndCheck(timeout) maybe?
 
 maybe so, but let's worry about that when we have other users of Process
 that need this feature in a generic parent class?
 
 I can't argue with that :). Feel free to leave them in the XServer
 object for now, though I'd prefer if you renamed them to something
 like KillAndCheck so it's obvious that it actually does something
 different than the parent class Kill method.
 
 but isn't that the point of polymorphism? your object will do the right
 thing, regardless of what class it happens to be at the time.
 
 The difference here is that we are doing two different things.
 Process::Kill() merely signals the process. XServer::KillAndCheck()
 signals the process and waits for it to die. Using polymorphism to
 hide implementation differences between classes is useful. Using
 polymorphism to hide behavior changes is dangerous.
 
 Let's say you define XServer::Kill(), so that it waits a second to
 check if it has died. Now, let's say there is a function that is
 passed a Process object and calls Kill() on it. If the object is
 simply a Process object, Kill() will return immediately after
 signalling the process. If the object is an XServer project, it will
 only return after waiting for up to a second. This could create odd
 interactions due to the hidden behavior difference depending on the
 actual type of the object.
 
 What qualifies as a behavior difference vs an implementation
 difference isn't black and white, but this particular case seems to
 me to clearly fall on the behavior side of the fence.

changed locally, will be in next patch set

Cheers,
  Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2012-07-09 Thread Chase Douglas 

On 07/08/2012 06:50 PM, Peter Hutterer wrote:

On Fri, Jul 06, 2012 at 11:08:13AM -0700, Chase Douglas wrote:

On 07/05/2012 05:50 PM, Peter Hutterer wrote:

On Thu, Jul 05, 2012 at 02:09:13PM -0700, Chase Douglas wrote:

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.hutte...@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.


I think we should keep Process basic. For most processes failing to kill it
should be an issue in itself, it's just the server that takes too long to
shut down and needs special handling.


I thought about that too. I think the question to ask is: do we
think most users or subclasses of Process are going to want wait for
the process to really die. The Process::Kill and Terminate methods
just try once to raise the signal to the child process. They return
true if the signal was sent, but don't check what happens after
that. Perhaps a better approach to resolving this issue would be to
add second versions of these methods that would take a timeout value
and return true only if the signal was successfully sent and the
process died.
KillAndCheck(timeout) and TerminateAndCheck(timeout) maybe?


maybe so, but let's worry about that when we have other users of Process
that need this feature in a generic parent class?


I can't argue with that :). Feel free to leave them in the XServer 
object for now, though I'd prefer if you renamed them to something like 
KillAndCheck so it's obvious that it actually does something different 
than the parent class Kill method.


-- Chase
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2012-07-09 Thread Peter Hutterer 
On Mon, Jul 09, 2012 at 10:19:50AM -0700, Chase Douglas wrote:
 On 07/08/2012 06:50 PM, Peter Hutterer wrote:
 On Fri, Jul 06, 2012 at 11:08:13AM -0700, Chase Douglas wrote:
 On 07/05/2012 05:50 PM, Peter Hutterer wrote:
 On Thu, Jul 05, 2012 at 02:09:13PM -0700, Chase Douglas wrote:
 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.hutte...@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.
 
 I think we should keep Process basic. For most processes failing to kill it
 should be an issue in itself, it's just the server that takes too long to
 shut down and needs special handling.
 
 I thought about that too. I think the question to ask is: do we
 think most users or subclasses of Process are going to want wait for
 the process to really die. The Process::Kill and Terminate methods
 just try once to raise the signal to the child process. They return
 true if the signal was sent, but don't check what happens after
 that. Perhaps a better approach to resolving this issue would be to
 add second versions of these methods that would take a timeout value
 and return true only if the signal was successfully sent and the
 process died.
 KillAndCheck(timeout) and TerminateAndCheck(timeout) maybe?
 
 maybe so, but let's worry about that when we have other users of Process
 that need this feature in a generic parent class?
 
 I can't argue with that :). Feel free to leave them in the XServer
 object for now, though I'd prefer if you renamed them to something
 like KillAndCheck so it's obvious that it actually does something
 different than the parent class Kill method.

but isn't that the point of polymorphism? your object will do the right
thing, regardless of what class it happens to be at the time.

Cheers,
  Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2012-07-09 Thread Chase Douglas 

On 07/09/2012 04:26 PM, Peter Hutterer wrote:

On Mon, Jul 09, 2012 at 10:19:50AM -0700, Chase Douglas wrote:

On 07/08/2012 06:50 PM, Peter Hutterer wrote:

On Fri, Jul 06, 2012 at 11:08:13AM -0700, Chase Douglas wrote:

On 07/05/2012 05:50 PM, Peter Hutterer wrote:

On Thu, Jul 05, 2012 at 02:09:13PM -0700, Chase Douglas wrote:

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.hutte...@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.


I think we should keep Process basic. For most processes failing to kill it
should be an issue in itself, it's just the server that takes too long to
shut down and needs special handling.


I thought about that too. I think the question to ask is: do we
think most users or subclasses of Process are going to want wait for
the process to really die. The Process::Kill and Terminate methods
just try once to raise the signal to the child process. They return
true if the signal was sent, but don't check what happens after
that. Perhaps a better approach to resolving this issue would be to
add second versions of these methods that would take a timeout value
and return true only if the signal was successfully sent and the
process died.
KillAndCheck(timeout) and TerminateAndCheck(timeout) maybe?


maybe so, but let's worry about that when we have other users of Process
that need this feature in a generic parent class?


I can't argue with that :). Feel free to leave them in the XServer
object for now, though I'd prefer if you renamed them to something
like KillAndCheck so it's obvious that it actually does something
different than the parent class Kill method.


but isn't that the point of polymorphism? your object will do the right
thing, regardless of what class it happens to be at the time.


The difference here is that we are doing two different things. 
Process::Kill() merely signals the process. XServer::KillAndCheck() 
signals the process and waits for it to die. Using polymorphism to hide 
implementation differences between classes is useful. Using polymorphism 
to hide behavior changes is dangerous.


Let's say you define XServer::Kill(), so that it waits a second to check 
if it has died. Now, let's say there is a function that is passed a 
Process object and calls Kill() on it. If the object is simply a Process 
object, Kill() will return immediately after signalling the process. If 
the object is an XServer project, it will only return after waiting for 
up to a second. This could create odd interactions due to the hidden 
behavior difference depending on the actual type of the object.


What qualifies as a behavior difference vs an implementation difference 
isn't black and white, but this particular case seems to me to clearly 
fall on the behavior side of the fence.


-- Chase
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2012-07-08 Thread Peter Hutterer 
On Fri, Jul 06, 2012 at 11:08:13AM -0700, Chase Douglas wrote:
 On 07/05/2012 05:50 PM, Peter Hutterer wrote:
 On Thu, Jul 05, 2012 at 02:09:13PM -0700, Chase Douglas wrote:
 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.hutte...@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.
 
 I think we should keep Process basic. For most processes failing to kill it
 should be an issue in itself, it's just the server that takes too long to
 shut down and needs special handling.
 
 I thought about that too. I think the question to ask is: do we
 think most users or subclasses of Process are going to want wait for
 the process to really die. The Process::Kill and Terminate methods
 just try once to raise the signal to the child process. They return
 true if the signal was sent, but don't check what happens after
 that. Perhaps a better approach to resolving this issue would be to
 add second versions of these methods that would take a timeout value
 and return true only if the signal was successfully sent and the
 process died.
 KillAndCheck(timeout) and TerminateAndCheck(timeout) maybe?

maybe so, but let's worry about that when we have other users of Process
that need this feature in a generic parent class?

Cheers,
  Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2012-07-06 Thread Chase Douglas 

On 07/05/2012 05:50 PM, Peter Hutterer wrote:

On Thu, Jul 05, 2012 at 02:09:13PM -0700, Chase Douglas wrote:

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.hutte...@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.


I think we should keep Process basic. For most processes failing to kill it
should be an issue in itself, it's just the server that takes too long to
shut down and needs special handling.


I thought about that too. I think the question to ask is: do we think 
most users or subclasses of Process are going to want wait for the 
process to really die. The Process::Kill and Terminate methods just try 
once to raise the signal to the child process. They return true if the 
signal was sent, but don't check what happens after that. Perhaps a 
better approach to resolving this issue would be to add second versions 
of these methods that would take a timeout value and return true only if 
the signal was successfully sent and the process died.

KillAndCheck(timeout) and TerminateAndCheck(timeout) maybe?

-- Chase
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2012-07-05 Thread 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.hutte...@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_castSuperclass(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;
+}




___

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

2012-07-05 Thread Peter Hutterer 
On Thu, Jul 05, 2012 at 02:09:13PM -0700, Chase Douglas wrote:
 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.hutte...@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.

I think we should keep Process basic. For most processes failing to kill it
should be an issue in itself, it's just the server that takes too long to
shut down and needs special handling.

 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_castSuperclass(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 :).

years of Java have skewed my understanding of inhertiance and I only wrapped
my head around the C++ way yesterday. I'll fix this up.

Cheers,
  Peter

 +
 +/**
* 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

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

2012-07-03 Thread Peter Hutterer 
Signed-off-by: Peter Hutterer peter.hutte...@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);
+
+/**
  * 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;
+}
-- 
1.7.10.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2012-07-03 Thread Chase Douglas 

On 07/02/2012 11:44 PM, Peter Hutterer wrote:

Signed-off-by: Peter Hutterer peter.hutte...@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.



+
+/**
   * 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@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

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

2012-07-03 Thread Peter Hutterer 
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.hutte...@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.

Cheers,
  Peter

 
 +
 +/**
* 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@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel