Re: [PATCH 1/3] support: Add support_process_state_wait

[Florian Weimer <fw at deneb dot enyo dot de>]

* Adhemerval Zanella:

> diff --git a/support/process_state.h b/support/process_state.h
> new file mode 100644
> index 0000000000..d983197247
> --- /dev/null
> +++ b/support/process_state.h
> @@ -0,0 +1,45 @@

> +#ifndef SUPPORT_PROCESS_STATE_H
> +#define SUPPORT_PROCESS_STATE_H
> +
> +#include <sys/types.h>

I think you also need to include <time.h> for struct timespec,
strictly speaking.

> diff --git a/support/support_process_state.c b/support/support_process_state.c
> new file mode 100644
> index 0000000000..16c2c3642e
> --- /dev/null
> +++ b/support/support_process_state.c

> +/* Returns the character representing the task state from FD.  */
> +static char find_state (int fd)
> +{
> +  char statusbuf[strlen ("Name:\t")
> +		 + 64                   /* task name.  */
> +		 + strlen ("Umask:\t")  /* optional.  */
> +		 + 4                    /* umask size.  */
> +		 + strlen ("State:\t")
> +		 + 1                    /* <state>, 1 char.  */
> +		 + 1];                  /* Final \0.  */
> +
> +  ssize_t ret = read (fd, statusbuf, sizeof (statusbuf) - 1);
> +  TEST_VERIFY (ret >= 0);
> +  statusbuf[ret] = '\0';
> +
> +  char *statestr = strstr (statusbuf, "State:\t");
> +  TEST_VERIFY (statestr != NULL);
> +  char state;
> +  int sret = sscanf (statestr, "%*s %c", &state);
> +  TEST_VERIFY (sret == 1);
> +
> +  return state;
> +}

I think the expectation here is that more things will be inserted
after Name:.  As you note, Umask: is already a late arrival.

So if simplicity is a concern, I would just read 4 KiB or so, and then
use strstr with "\nState:\t" (note the \n).  The sscanf seems
superfluous in this context.  Every else heads in the direction of
getline …

I think it also makes sense to return 0 in case the state cannot be
determined, rather than an indeterminate value.

> +void
> +support_process_state_wait (pid_t pid, enum support_process_state state,
> +			    struct timespec *poll_ts)
> +{
> +#ifdef __linux__
> +  /* For Linux it does a polling check on /proc/<pid>/status checking on
> +     third field.  */
> +
> +  /* It mimics the kernel states from fs/proc/array.c  */
> +  static const struct process_states_t
> +  {
> +    enum support_process_state s;
> +    char v;
> +  } process_states[] = {
> +    { support_process_state_running,      'R' },
> +    { support_process_state_sleeping,     'S' },
> +    { support_process_state_disk_sleep,   'D' },
> +    { support_process_state_stopped,      'T' },
> +    { support_process_state_tracing_stop, 't' },
> +    { support_process_state_dead,         'X' },
> +    { support_process_state_zombie,       'Z' },
> +    { support_process_state_parked,       'P' },
> +  };
> +
> +  char proc_path[sizeof ("/proc/" + 3) * sizeof (pid_t) + sizeof ("/stat")
> +		 + 1];
> +  snprintf (proc_path, sizeof (proc_path), "/proc/%i/status", pid);
> +
> +  int fd = xopen (proc_path, O_RDONLY, 0600);
> +
> +  for (;;)
> +    {
> +      char cur_state = find_state (fd);
> +
> +      for (size_t i = 0; i < array_length (process_states); ++i)
> +	if (state & process_states[i].s && cur_state == process_states[i].v)
> +	  goto found;

I missed that before, I think we should error out if the state is
completely unknown.  Maybe use strchr on "RSDTtXZP" instead, and use
that for the shift? 8-)