From patchwork Thu Dec 26 14:20:37 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 37091 Received: (qmail 46638 invoked by alias); 26 Dec 2019 14:20:48 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 46623 invoked by uid 89); 26 Dec 2019 14:20:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=STATE, simplicity, arrival, tracing X-HELO: mail-pj1-f67.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:cc:references:from:autocrypt:subject:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=Dclaf6TPYRlHJKF2lK1ifX4/zp492E9bSD8lZ8z+Kms=; b=jndq0QBPKJWkz/zYFo94H86N+xkxQlDSfzJwPNrdHYj14S+2KBmjlUozpUB7l/KR5+ FW6+49yDkdGOMDULMdWVHDza3D0lxsJxeEEteZaXXrKihTfBVlPV2s69y4QoiEmL1ooh chS8u5ggdSaNQjHlvSJF8EWY+KfBtlG1EnYv22/MkU/yfVGnr7BTo8bvQhyqBlzRtfpD llkwHJS4wWkayUV9/079FRd+r5t/HuBSDEnz36qkmfReUd5mpmuuuwaZp7BLaRRdhw8R 1KUhCTgppd3C+LJ7AsF+vw1RPIPFIG2/P9JCm5O+uXZGg3sgZLl/Rxt0dpFRqnoEQuEP 3GpQ== Return-Path: To: Florian Weimer Cc: Florian Weimer , libc-alpha@sourceware.org References: <20191120141535.5303-1-adhemerval.zanella@linaro.org> <87bls3hywz.fsf@oldenburg2.str.redhat.com> <7bb7f49e-084f-cd65-ad02-48660ffeae5c@linaro.org> <87r20yn3g1.fsf@mid.deneb.enyo.de> From: Adhemerval Zanella Subject: Re: [PATCH 1/3] support: Add support_process_state_wait Message-ID: <5bd5cb39-1906-bb76-13f9-c7d2dc31b68d@linaro.org> Date: Thu, 26 Dec 2019 11:20:37 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <87r20yn3g1.fsf@mid.deneb.enyo.de> 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 > > I think you also need to include 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 /* , 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//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). diff --git a/support/Makefile b/support/Makefile index 23c6d74627..5a3bdd8e9f 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 \ @@ -95,6 +96,7 @@ libsupport-routines = \ xfopen \ xfork \ xftruncate \ + xgetline \ xgetsockname \ xlisten \ xlseek \ @@ -223,6 +225,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..4325909efd --- /dev/null +++ b/support/process_state.h @@ -0,0 +1,47 @@ +/* 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 + . */ + +#ifndef SUPPORT_PROCESS_STATE_H +#define SUPPORT_PROCESS_STATE_H + +#include + +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). + For an invalid state not represented by SUPPORT_PROCESS_STATE, it fallbacks + to a 2 second sleep. */ +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..6a6dbc6287 --- /dev/null +++ b/support/support_process_state.c @@ -0,0 +1,91 @@ +/* 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 + . */ + +#include +#include +#include + +#include + +#include +#include +#include + +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//status checking on + third field. */ + + /* It mimics the kernel states from fs/proc/array.c */ + static struct process_states + { + 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 spath[sizeof ("/proc/" + 3) * sizeof (pid_t) + sizeof ("/status") + 1]; + snprintf (spath, sizeof (spath), "/proc/%i/status", pid); + + FILE *fstatus = xfopen (spath, "r"); + char *line = NULL; + size_t linesiz = 0; + + for (;;) + { + char cur_state = -1; + while (xgetline (&line, &linesiz, fstatus) != -1) + if (strncmp (line, "State:", strlen ("State:")) == 0) + { + TEST_COMPARE (sscanf (line, "%*s %c", &cur_state), 1); + break; + } + if (cur_state == -1) + break; + + for (size_t i = 0; i < array_length (process_states); ++i) + if (state & process_states[i].s && cur_state == process_states[i].v) + { + free (line); + xfclose (fstatus); + return; + } + + rewind (fstatus); + fflush (fstatus); + + nanosleep (poll_ts ? poll_ts : &(struct timespec) { 0, 10000000 }, NULL); + } + + free (line); + xfclose (fstatus); + /* Fallback to nanosleep if an invalid state is found. */ +#endif + nanosleep (&(struct timespec) { 2, 0 }, NULL); +} diff --git a/support/tst-support-process_state.c b/support/tst-support-process_state.c new file mode 100644 index 0000000000..3615bbe192 --- /dev/null +++ b/support/tst-support-process_state.c @@ -0,0 +1,105 @@ +/* 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 + . */ + +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#ifndef WEXITED +# define WEXITED 0 +#endif + +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 diff --git a/support/xgetline.c b/support/xgetline.c new file mode 100644 index 0000000000..5b010bcf56 --- /dev/null +++ b/support/xgetline.c @@ -0,0 +1,34 @@ +/* fopen with error checking. + Copyright (C) 2016-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 + . */ + +#include + +#include +#include + +ssize_t +xgetline (char **lineptr, size_t *n, FILE *stream) +{ + int old_errno = errno; + errno = 0; + ssize_t ret = getline (lineptr, n, stream); + if (ret == -1 && errno != 0) + FAIL_EXIT1 ("getline failed: %m"); + errno = old_errno; + return ret; +} diff --git a/support/xstdio.h b/support/xstdio.h index e4e52b4c7e..df6ee27711 100644 --- a/support/xstdio.h +++ b/support/xstdio.h @@ -27,6 +27,8 @@ __BEGIN_DECLS FILE *xfopen (const char *path, const char *mode); void xfclose (FILE *); +ssize_t xgetline (char **lineptr, size_t *n, FILE *stream); + __END_DECLS #endif /* SUPPORT_XSTDIO_H */