Re: [PATCH v2 xorg-gtest] process: add state enum and GetState()

2012-08-14 Thread Chase Douglas

On 08/13/2012 06:20 PM, Peter Hutterer wrote:

Add a set of basic states to Process to allow callers to keep track of which
state a process is in (as seen from the library). This simplifies code that
needs to happen on certain conditions only, e.g. log file cleanup is only
needed if the process was previously started.

Signed-off-by: Peter Hutterer 
---
Changes to v1:
- Move enum to Process class to better confine it
- Check state on GetState(), update if it terminated on its own.
- Check state on KillSelf(), return true if it already terminated anyway

  include/xorg/gtest/xorg-gtest-process.h | 17 +
  src/process.cpp | 25 +
  2 files changed, 42 insertions(+)

diff --git a/include/xorg/gtest/xorg-gtest-process.h 
b/include/xorg/gtest/xorg-gtest-process.h
index 402be49..c4b2309 100644
--- a/include/xorg/gtest/xorg-gtest-process.h
+++ b/include/xorg/gtest/xorg-gtest-process.h
@@ -62,6 +62,16 @@ namespace testing {
   */
  class Process {
   public:
+   /**
+* Describes the state of a process.
+*/
+   enum State {
+ ERROR,/**< An error has occured, state is now unknown */
+ NONE, /**< The process has not been started yet */
+ RUNNING,  /**< The process has been started */
+ TERMINATED,   /**< The process was terminated by this library */
+   };
+
/**
 * Helper function to adjust the environment of the current process.
 *
@@ -179,6 +189,13 @@ class Process {
 */
pid_t Pid() const;

+  /**
+   * Return the state of the process.
+   *
+   * @return The current state of the process
+   */
+  enum Process::State GetState();
+
   private:
struct Private;
std::auto_ptr d_;
diff --git a/src/process.cpp b/src/process.cpp
index 7df2b84..c976720 100644
--- a/src/process.cpp
+++ b/src/process.cpp
@@ -42,10 +42,26 @@

  struct xorg::testing::Process::Private {
pid_t pid;
+  enum State state;
  };

  xorg::testing::Process::Process() : d_(new Private) {
d_->pid = -1;
+  d_->state = NONE;
+}
+
+enum xorg::testing::Process::State xorg::testing::Process::GetState() {
+  if (d_->state == RUNNING) {
+int status;
+int pid = waitpid(Pid(), &status, WNOHANG);
+if (pid == Pid()) {
+  if (WIFEXITED(status))
+d_->pid = -1;
+d_->state = TERMINATED;


This will mark the process as having terminated cleanly, even if it 
actually died due to an error. In fact, under the assumption that any 
process is meant to be indefinitely long, any exit first noticed here is 
an error.


Should we set state here to ERROR instead? In the successful path, we 
will end up setting the state to TERMINATED inside KillSelf().



+}
+  }
+
+  return d_->state;
  }

  void xorg::testing::Process::Start(const std::string &program, const 
std::vector &argv) {
@@ -55,6 +71,7 @@ void xorg::testing::Process::Start(const std::string 
&program, const std::vector
d_->pid = fork();

if (d_->pid == -1) {
+d_->state = ERROR;
  throw std::runtime_error("Failed to fork child process");
} else if (d_->pid == 0) { /* Child */
  close(0);
@@ -73,8 +90,11 @@ void xorg::testing::Process::Start(const std::string 
&program, const std::vector

  execvp(program.c_str(), &args[0]);

+d_->state = ERROR;
  throw std::runtime_error("Failed to start process");
}
+
+  d_->state = RUNNING;
  }

  void xorg::testing::Process::Start(const std::string& program, va_list args) {
@@ -112,6 +132,9 @@ bool xorg::testing::Process::WaitForExit(unsigned int 
timeout) {
  bool xorg::testing::Process::KillSelf(int signal, unsigned int timeout) {
bool wait_success = true;

+  if (GetState() == TERMINATED)
+return true;
+


This would then be:
State state = GetState();
if (state == TERMINATED)
return true;
if (state == ERROR)
return false;


if (d_->pid == -1) {
  return false;
} else if (d_->pid == 0) {
@@ -120,12 +143,14 @@ bool xorg::testing::Process::KillSelf(int signal, 
unsigned int timeout) {
} else { /* Parent */
  if (kill(d_->pid, signal) < 0) {
d_->pid = -1;
+  d_->state = ERROR;
return false;
  }
  if (timeout > 0)
wait_success = WaitForExit(timeout);
  d_->pid = -1;
}
+  d_->state = TERMINATED;
return wait_success;
  }




___
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


[PATCH v2 xorg-gtest] process: add state enum and GetState()

2012-08-13 Thread Peter Hutterer
Add a set of basic states to Process to allow callers to keep track of which
state a process is in (as seen from the library). This simplifies code that
needs to happen on certain conditions only, e.g. log file cleanup is only
needed if the process was previously started.

Signed-off-by: Peter Hutterer 
---
Changes to v1:
- Move enum to Process class to better confine it
- Check state on GetState(), update if it terminated on its own. 
- Check state on KillSelf(), return true if it already terminated anyway

 include/xorg/gtest/xorg-gtest-process.h | 17 +
 src/process.cpp | 25 +
 2 files changed, 42 insertions(+)

diff --git a/include/xorg/gtest/xorg-gtest-process.h 
b/include/xorg/gtest/xorg-gtest-process.h
index 402be49..c4b2309 100644
--- a/include/xorg/gtest/xorg-gtest-process.h
+++ b/include/xorg/gtest/xorg-gtest-process.h
@@ -62,6 +62,16 @@ namespace testing {
  */
 class Process {
  public:
+   /**
+* Describes the state of a process.
+*/
+   enum State {
+ ERROR,/**< An error has occured, state is now unknown */
+ NONE, /**< The process has not been started yet */
+ RUNNING,  /**< The process has been started */
+ TERMINATED,   /**< The process was terminated by this library */
+   };
+
   /**
* Helper function to adjust the environment of the current process.
*
@@ -179,6 +189,13 @@ class Process {
*/
   pid_t Pid() const;
 
+  /**
+   * Return the state of the process.
+   *
+   * @return The current state of the process
+   */
+  enum Process::State GetState();
+
  private:
   struct Private;
   std::auto_ptr d_;
diff --git a/src/process.cpp b/src/process.cpp
index 7df2b84..c976720 100644
--- a/src/process.cpp
+++ b/src/process.cpp
@@ -42,10 +42,26 @@
 
 struct xorg::testing::Process::Private {
   pid_t pid;
+  enum State state;
 };
 
 xorg::testing::Process::Process() : d_(new Private) {
   d_->pid = -1;
+  d_->state = NONE;
+}
+
+enum xorg::testing::Process::State xorg::testing::Process::GetState() {
+  if (d_->state == RUNNING) {
+int status;
+int pid = waitpid(Pid(), &status, WNOHANG);
+if (pid == Pid()) {
+  if (WIFEXITED(status))
+d_->pid = -1;
+d_->state = TERMINATED;
+}
+  }
+
+  return d_->state;
 }
 
 void xorg::testing::Process::Start(const std::string &program, const 
std::vector &argv) {
@@ -55,6 +71,7 @@ void xorg::testing::Process::Start(const std::string 
&program, const std::vector
   d_->pid = fork();
 
   if (d_->pid == -1) {
+d_->state = ERROR;
 throw std::runtime_error("Failed to fork child process");
   } else if (d_->pid == 0) { /* Child */
 close(0);
@@ -73,8 +90,11 @@ void xorg::testing::Process::Start(const std::string 
&program, const std::vector
 
 execvp(program.c_str(), &args[0]);
 
+d_->state = ERROR;
 throw std::runtime_error("Failed to start process");
   }
+
+  d_->state = RUNNING;
 }
 
 void xorg::testing::Process::Start(const std::string& program, va_list args) {
@@ -112,6 +132,9 @@ bool xorg::testing::Process::WaitForExit(unsigned int 
timeout) {
 bool xorg::testing::Process::KillSelf(int signal, unsigned int timeout) {
   bool wait_success = true;
 
+  if (GetState() == TERMINATED)
+return true;
+
   if (d_->pid == -1) {
 return false;
   } else if (d_->pid == 0) {
@@ -120,12 +143,14 @@ bool xorg::testing::Process::KillSelf(int signal, 
unsigned int timeout) {
   } else { /* Parent */
 if (kill(d_->pid, signal) < 0) {
   d_->pid = -1;
+  d_->state = ERROR;
   return false;
 }
 if (timeout > 0)
   wait_success = WaitForExit(timeout);
 d_->pid = -1;
   }
+  d_->state = TERMINATED;
   return wait_success;
 }
 
-- 
1.7.11.2

___
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