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

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

> It allows parent process to wait for children state using a polling
> strategy over procfs on Linux.  The polling is used over ptrace to
> avoid the need to handle signals on the target pid and to handle some
> system specific limitations (such as YAMA).
>
> The polling has some limitations, such as resource consumption due
> the procfs read in a loop and the lack of synchronization after the
> state is obtained.
>
> The interface idea is to simplify some sleep synchronization in waitid
> tests.

The goal is to reduce timeouts, I assume, replacing arbitrary two-second
waits.

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

> +enum process_state_t
> +{
> +  process_state_running      = 0x01,  /* R (running)  */
> +  process_state_sleeping     = 0x02,  /* S (sleeping)  */
> +  process_state_disk_sleep   = 0x04,  /* D (disk sleep)  */
> +  process_state_stopped      = 0x08,  /* T (stopped)  */
> +  process_state_tracing_stop = 0x10,  /* t (tracing stop)  */
> +  process_state_dead         = 0x20,  /* X (dead)  */
> +  process_state_zombie       = 0x40,  /* Z (zombie)  */
> +  process_state_parked       = 0x80,  /* P (parked)  */
> +};

Maybe drop the reversed _t suffix and add a support_ prefix?

> +
> +/* Wait for process PID to reach state STATE.  It can be a combination of
> +   multiple possible states ('process_state_running | process_state_sleeping')
> +   where the function return when any of these state are observed.  */
> +void support_process_state_wait (pid_t pid, enum process_state_t state);

This interface does not allow to poll for a process not to have a
specific state.  I guess we can adjust it once the need arises, so you
can leave it for now.

> --- /dev/null
> +++ b/support/support_process_state.c
> @@ -0,0 +1,89 @@

> +void
> +support_process_state_wait (pid_t pid, enum process_state_t state)
> +{
> +#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 struct process_states_t
> +  {
> +    enum process_state_t s;
> +    char v;
> +  } process_states[] = {
> +    { process_state_running,      'R' },
> +    { process_state_sleeping,     'S' },
> +    { process_state_disk_sleep,   'D' },
> +    { process_state_stopped,      'T' },
> +    { process_state_tracing_stop, 't' },
> +    { process_state_dead,         'X' },
> +    { process_state_zombie,       'Z' },
> +    { process_state_parked,       'P' },
> +  };

Should be const.

> +  char proc_path[sizeof ("/proc/" + 3) * sizeof (pid_t) + sizeof ("/stat")
> +		 + 1];
> +  snprintf (proc_path, sizeof (proc_path), "/proc/%i/stat", pid);
> +
> +  int fd = xopen (proc_path, O_RDONLY, 0600);
> +
> +  char statbuf[3 * sizeof (pid_t)  /* pid.  */
> +               + 2                 /* ' (' */
> +               + 64                /* task name.  */
> +               + 2                 /* ' )'  */
> +               + 1                 /* <state>, 1 char.  */
> +               + 1];               /* Final \0.  */
> +  for (;;)
> +    {
> +      ssize_t ret = read (fd, statbuf, sizeof (statbuf) - 1);
> +      TEST_VERIFY (ret >= 0);
> +      statbuf[ret] = '\0';
> +
> +      char cur_state;
> +      int sret = sscanf (statbuf, "%*i %*s %c", &cur_state);
> +      TEST_VERIFY (sret == 1);

Hmm.  Does this work if COMM includes a space?  I think it's safer to
parse /proc/PID/status.

> +      for (size_t i = 0; i < array_length (process_states); ++i)
> +	if (state & process_states[i].s && cur_state == process_states[i].v)
> +	  {
> +	    close (fd);
> +	    return;
> +	  }
> +
> +      xlseek (fd, 0, SEEK_SET);
> +      nanosleep (&(struct timespec) { 0, 10000000 }, NULL);
> +    }
> +
> +  close (fd);

Should this have a fallback timeout?

The sleep should probably be usleep or nanosleep, to avoid interfering
with laram.

The test looks reasonable.

Thanks,
Florian