[1/3] support: Add support_process_state_wait
Commit Message
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
* 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-)
@@ -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 \
new file mode 100644
@@ -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
new file mode 100644
@@ -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
+}
new file mode 100644
@@ -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>