[1/3] support: Add support_process_state_wait

  On 20/12/2019 17:32, Florian Weimer wrote:
> * 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.

Ack.

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

I was trying more to make support_process_state_wait not consume much
resources, but it seems since /proc/pid/status does not have a
stable format we will need to parse it in the end.  I have adjusted
to use getline.

> 
>> +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-)
> 

What I did instead is to to fallback to nanosleep (2s) for such case
(another possibility would be to just assert/abort).

--

[1/3] support: Add support_process_state_wait

Commit Message

Patch