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

[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 <peter.hutterer at who-t.net>
> ---
> 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<Private> 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<std::string> &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;
>   }
>
>