[ptest-runner,4/4] utils.c: wait_child reimplement timeout using alarm

Submitted by Aníbal Limón on March 23, 2021, 2:10 a.m. | Patch ID: 179395

Details

Message ID 20210323021038.1049654-4-anibal.limon@linaro.org
State New
Headers show

Commit Message

Aníbal Limón March 23, 2021, 2:10 a.m.
Since we are using threads to read from child, no complex logic is
needed for handle timeouts use alarm(2).

[YOCTO #14220]

Signed-off-by: Aníbal Limón <anibal.limon@linaro.org>
---
 utils.c | 60 ++++++++++++++++++++-------------------------------------
 1 file changed, 21 insertions(+), 39 deletions(-)

Patch hide | download patch | download mbox

diff --git a/utils.c b/utils.c
index 84cb570..1a3c90f 100644
--- a/utils.c
+++ b/utils.c
@@ -57,6 +57,9 @@ 
 static struct {
 	int fds[2];
 	FILE *fps[2];
+
+	int timeouted;
+	pid_t pid;
 } _child_reader;
 
 static inline char *
@@ -335,45 +338,25 @@  run_child(char *run_ptest, int fd_stdout, int fd_stderr)
 	/* exit(1); not needed? */
 }
 
-static inline int
-wait_child(pid_t pid, int timeout, int *timeouted)
+static void
+timeout_child_handler(int signo)
 {
-	struct timespec sentinel;
-	clockid_t clock = CLOCK_MONOTONIC;
-	int looping = 1;
+	_child_reader.timeouted = 1;
+	kill(-_child_reader.pid, SIGKILL);
+}
 
+static inline int
+wait_child(pid_t pid, int timeout)
+{
 	int status = -1;
-	int waitflags;
-
-	if (clock_gettime(clock, &sentinel) == -1) {
-		clock = CLOCK_REALTIME;
-		clock_gettime(clock, &sentinel);
-	}
-
-	*timeouted = 0;
 
-	while (looping) {
-		waitflags = WNOHANG;
+	_child_reader.timeouted = 0;
+	_child_reader.pid = pid;
 
-		if (timeout >= 0) {
-			struct timespec time;
-
-			clock_gettime(clock, &time);
-			if ((time.tv_sec - sentinel.tv_sec) > timeout) {
-				*timeouted = 1;
-				kill(-pid, SIGKILL);
-				waitflags = 0;
-			}
-		}
-
-		if (waitpid(pid, &status, waitflags) == pid)
-			looping = 0;
-
-		clock_gettime(clock, &sentinel);
-
-		if (WIFEXITED(status))
-			status = WEXITSTATUS(status);
-	}
+	alarm(timeout);
+	waitpid(pid, &status, 0);
+	if (WIFEXITED(status))
+		status = WEXITSTATUS(status);
 
 	return status;
 }
@@ -438,7 +421,6 @@  run_ptests(struct ptest_list *head, const struct ptest_options opts,
 	pid_t child;
 	int pipefd_stdout[2];
 	int pipefd_stderr[2];
-	int timeouted;
 	time_t sttime, entime;
 	time_t duration;
 	int slave;
@@ -477,6 +459,7 @@  run_ptests(struct ptest_list *head, const struct ptest_options opts,
 			close(pipefd_stdout[1]);
 			break;
 		}
+		signal(SIGALRM, timeout_child_handler);
 
 		fprintf(fp, "START: %s\n", progname);
 		PTEST_LIST_ITERATE_START(head, p)
@@ -527,8 +510,7 @@  run_ptests(struct ptest_list *head, const struct ptest_options opts,
 				fprintf(fp, "BEGIN: %s\n", ptest_dir);
 
 
-				status = wait_child(child, opts.timeout, &timeouted);
-
+				status = wait_child(child, opts.timeout);
 
 				entime = time(NULL);
 				duration = entime - sttime;
@@ -538,11 +520,11 @@  run_ptests(struct ptest_list *head, const struct ptest_options opts,
 					rc += 1;
 				}
 				fprintf(fp, "DURATION: %d\n", (int) duration);
-				if (timeouted)
+				if (_child_reader.timeouted)
 					fprintf(fp, "TIMEOUT: %s\n", ptest_dir);
 
 				if (opts.xml_filename)
-					xml_add_case(xh, status, ptest_dir, timeouted, (int) duration);
+					xml_add_case(xh, status, ptest_dir, _child_reader.timeouted, (int) duration);
 
 				fprintf(fp, "END: %s\n", ptest_dir);
 				fprintf(fp, "%s\n", get_stime(stime, GET_STIME_BUF_SIZE, entime));

Comments

Yi Fan Yu March 24, 2021, 3:56 p.m.
On 3/22/21 10:10 PM, Aníbal Limón wrote:
> Since we are using threads to read from child, no complex logic is
> needed for handle timeouts use alarm(2).
TY for the patches.

This would change its behaviour to timeout exactly at X seconds.

New behaviour: if test exceeds X seconds, gets killed.
Old behaviour: if test exceeds X seconds with no output, get killed. If 
output is detected X-1 seconds, the timeout timer resets.
so a program could take 2X seconds and not get killed


Comment about the other patches:

We don't even need an extra thread because before the wait_for_child had 
to do 2 things:

* read a pipe and output that pipe to stdout (child_reader does that)
* check for timeout (sigalarm takes care of that now)



last thing, can you push this to a branch (let's say) master-next, so I 
can test it with a full ptest run.

yifan
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#52838): https://lists.yoctoproject.org/g/yocto/message/52838
Mute This Topic: https://lists.yoctoproject.org/mt/81542216/3617530
Group Owner: yocto+owner@lists.yoctoproject.org
Unsubscribe: https://lists.yoctoproject.org/g/yocto/unsub [oe-patchwork@oe-patch.openembedded.org]
-=-=-=-=-=-=-=-=-=-=-=-
Aníbal Limón March 24, 2021, 6:30 p.m.
On Wed, 24 Mar 2021 at 09:56, Yi Fan Yu <yifan.yu@windriver.com> wrote:

> On 3/22/21 10:10 PM, Aníbal Limón wrote:
> > Since we are using threads to read from child, no complex logic is
> > needed for handle timeouts use alarm(2).
> TY for the patches.
>
> This would change its behaviour to timeout exactly at X seconds.
>
> New behaviour: if test exceeds X seconds, gets killed.
> Old behaviour: if test exceeds X seconds with no output, get killed. If
> output is detected X-1 seconds, the timeout timer resets.
> so a program could take 2X seconds and not get killed
>

Yeah, thanks for remind me the old behavior, some years and I forget about
it, I will fix it and put certain comments about that.


>
>
> Comment about the other patches:
>
> We don't even need an extra thread because before the wait_for_child had
> to do 2 things:
>
> * read a pipe and output that pipe to stdout (child_reader does that)
> * check for timeout (sigalarm takes care of that now)
>
>
>
> last thing, can you push this to a branch (let's say) master-next, so I
> can test it with a full ptest run.
>

I pushed to dev branch,

http://git.yoctoproject.org/cgit/cgit.cgi/ptest-runner2/log/?h=dev

Regards,
Anibal


>
> yifan
>
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#52844): https://lists.yoctoproject.org/g/yocto/message/52844
Mute This Topic: https://lists.yoctoproject.org/mt/81542216/3617530
Group Owner: yocto+owner@lists.yoctoproject.org
Unsubscribe: https://lists.yoctoproject.org/g/yocto/unsub [oe-patchwork@oe-patch.openembedded.org]
-=-=-=-=-=-=-=-=-=-=-=-