diff mbox

[1/3] support: Add support_process_state_wait

Message ID 7bb7f49e-084f-cd65-ad02-48660ffeae5c@linaro.org
State Dropped
Headers show

Commit Message

Adhemerval Zanella Dec. 20, 2019, 8:19 p.m. UTC
On 20/12/2019 11:09, Florian Weimer wrote:
> * 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.

Indeed, I updated the commit message.

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

Ack.

> 
>> +
>> +/* 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.

Ok.

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

Ack.

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

It does not, I updated 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?

Do you mean if it can not get the process state? Currently
support_process_state_wait fails either if it can not open the status
file or if it can't find 'State:' field. Should we add a fallback for
each case?

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

Ok.

> 
> The test looks reasonable.


Updated patch below:

--

Comments

Florian Weimer Dec. 20, 2019, 8:32 p.m. UTC | #1
* 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-)
diff mbox

Patch

diff --git a/support/Makefile b/support/Makefile
index 23c6d74627..b0d28200af 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -56,6 +56,7 @@  libsupport-routines = \
   support_format_hostent \
   support_format_netent \
   support_isolate_in_subprocess \
+  support_process_state \
   support_ptrace \
   support_openpty \
   support_paths \
@@ -223,6 +224,7 @@  tests = \
   tst-support_capture_subprocess \
   tst-support_descriptors \
   tst-support_format_dns_packet \
+  tst-support-process_state \
   tst-support_quote_blob \
   tst-support_quote_string \
   tst-support_record_failure \
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 @@ 
+/* Wait for process state.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef SUPPORT_PROCESS_STATE_H
+#define SUPPORT_PROCESS_STATE_H
+
+#include <sys/types.h>
+
+enum support_process_state
+{
+  support_process_state_running      = 0x01,  /* R (running).  */
+  support_process_state_sleeping     = 0x02,  /* S (sleeping).  */
+  support_process_state_disk_sleep   = 0x04,  /* D (disk sleep).  */
+  support_process_state_stopped      = 0x08,  /* T (stopped).  */
+  support_process_state_tracing_stop = 0x10,  /* t (tracing stop).  */
+  support_process_state_dead         = 0x20,  /* X (dead).  */
+  support_process_state_zombie       = 0x40,  /* Z (zombie).  */
+  support_process_state_parked       = 0x80,  /* P (parked).  */
+};
+
+/* 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.
+   The timeout POLL_TS might be used to wait before polling the PID status
+   information from the kernel.  A NULL value will set to use a default
+   value (10000000 ns).  */
+void support_process_state_wait (pid_t pid, enum support_process_state state,
+				 struct timespec *poll_ts);
+
+#endif
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
@@ -0,0 +1,102 @@ 
+/* Wait for process state.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <fcntl.h>
+#include <time.h>
+#include <string.h>
+
+#include <array_length.h>
+
+#include <support/process_state.h>
+#include <support/xunistd.h>
+#include <support/check.h>
+
+/* 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;
+}
+
+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;
+
+      xlseek (fd, 0, SEEK_SET);
+
+      nanosleep (poll_ts ? poll_ts : &(struct timespec) { 0, 10000000 }, NULL);
+    }
+
+found:
+  xclose (fd);
+#else
+  nanosleep (&(struct timespec) { 2, 0 }, NULL);
+#endif
+}
diff --git a/support/tst-support-process_state.c b/support/tst-support-process_state.c
new file mode 100644
index 0000000000..d0aa142c76
--- /dev/null
+++ b/support/tst-support-process_state.c
@@ -0,0 +1,101 @@ 
+/* Wait for process state tests.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <errno.h>
+
+#include <support/test-driver.h>
+#include <support/process_state.h>
+#include <support/check.h>
+#include <support/xsignal.h>
+#include <support/xunistd.h>
+
+static void
+sigusr1_handler (int signo)
+{
+}
+
+static void
+test_child (void)
+{
+  xsignal (SIGUSR1, sigusr1_handler);
+
+  raise (SIGSTOP);
+
+  TEST_COMPARE (pause (), -1);
+  TEST_COMPARE (errno, EINTR);
+
+  while (1)
+    asm ("");
+}
+
+static int
+do_test (void)
+{
+  pid_t pid = xfork ();
+  if (pid == 0)
+    {
+      test_child ();
+      _exit (127);
+    }
+
+  /* Adding process_state_tracing_stop ('t') allows the test to work under
+     trace programs such as ptrace.  */
+  enum support_process_state stop_state = support_process_state_stopped
+				    | support_process_state_tracing_stop;
+
+  if (test_verbose)
+    printf ("info: waiting pid %d, state_stopped/state_tracing_stop\n",
+	    (int) pid);
+  support_process_state_wait (pid, stop_state, NULL);
+
+  if (kill (pid, SIGCONT) != 0)
+    FAIL_RET ("kill (%d, SIGCONT): %m\n", pid);
+
+  if (test_verbose)
+    printf ("info: waiting pid %d, state_sleeping\n", (int) pid);
+  support_process_state_wait (pid, support_process_state_sleeping, NULL);
+
+  if (kill (pid, SIGUSR1) != 0)
+    FAIL_RET ("kill (%d, SIGUSR1): %m\n", pid);
+
+  if (test_verbose)
+    printf ("info: waiting pid %d, state_running\n", (int) pid);
+  support_process_state_wait (pid, support_process_state_running, NULL);
+
+  if (kill (pid, SIGKILL) != 0)
+    FAIL_RET ("kill (%d, SIGKILL): %m\n", pid);
+
+  if (test_verbose)
+    printf ("info: waiting pid %d, state_zombie\n", (int) pid);
+  support_process_state_wait (pid, support_process_state_zombie, NULL);
+
+  siginfo_t info;
+  int r = waitid (P_PID, pid, &info, WEXITED);
+  TEST_COMPARE (r, 0);
+  TEST_COMPARE (info.si_signo, SIGCHLD);
+  TEST_COMPARE (info.si_code, CLD_KILLED);
+  TEST_COMPARE (info.si_status, SIGKILL);
+  TEST_COMPARE (info.si_pid, pid);
+
+  return 0;
+}
+
+#include <support/test-driver.c>