Skip to contentSkip to conversation

runner/executor: refactor error handling

PATCH In review Simon Ser 3 participants 3 messages Mar 20, 2019 – Mar 25, 2019

lore
Share & tools
lore

Simon Ser

author
#0 lore raw

PATCH v2 1/1 runner/executor: refactor error handling

* Refactor to use goto error handling
* Make execute_test_process noreturn to remove uninitialized variable warning
* Check fork() return value

---
 runner/executor.c | 68 ++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 33 deletions(-)
runner/executor.c +35 -33
index d535c276..289fe498 100644
@@ -753,9 +753,10 @@ static int monitor_output(pid_t child,
 	return killed;
 }
 
-static void execute_test_process(int outfd, int errfd,
-				 struct settings *settings,
-				 struct job_list_entry *entry)
+static void __attribute__((noreturn))
+execute_test_process(int outfd, int errfd,
+		     struct settings *settings,
+		     struct job_list_entry *entry)
 {
 	char *argv[4] = {};
 	size_t rootlen;
@@ -871,6 +872,7 @@ static int execute_next_entry(struct execute_state *state,
 	sigset_t mask;
 	int outpipe[2] = { -1, -1 };
 	int errpipe[2] = { -1, -1 };
+	int outfd, errfd;
 	char name[32];
 	pid_t child;
 	int result;
@@ -884,9 +886,9 @@ static int execute_next_entry(struct execute_state *state,
 	}
 
 	if (!open_output_files(dirfd, outputs, true)) {
-		close(dirfd);
 		fprintf(stderr, "Error opening output files\n");
-		return -1;
+		result = -1;
+		goto out_dirfd;
 	}
 
 	if (settings->sync) {
@@ -895,14 +897,9 @@ static int execute_next_entry(struct execute_state *state,
 	}
 
 	if (pipe(outpipe) || pipe(errpipe)) {
-		close_outputs(outputs);
-		close(dirfd);
-		close(outpipe[0]);
-		close(outpipe[1]);
-		close(errpipe[0]);
-		close(errpipe[1]);
 		fprintf(stderr, "Error creating pipes: %s\n", strerror(errno));
-		return -1;
+		result = -1;
+		goto out_pipe;
 	}
 
 	if ((kmsgfd = open("/dev/kmsg", O_RDONLY | O_CLOEXEC)) < 0) {
@@ -923,14 +920,8 @@ static int execute_next_entry(struct execute_state *state,
 	if (sigfd < 0) {
 		/* TODO: Handle better */
 		fprintf(stderr, "Cannot monitor child process with signalfd\n");
-		close(outpipe[0]);
-		close(errpipe[0]);
-		close(outpipe[1]);
-		close(errpipe[1]);
-		close(kmsgfd);
-		close_outputs(outputs);
-		close(dirfd);
-		return -1;
+		result = -1;
+		goto out_kmsgfd;
 	}
 
 	if (settings->log_level >= LOG_LEVEL_NORMAL) {
@@ -951,21 +942,17 @@ static int execute_next_entry(struct execute_state *state,
 	 * Flush outputs before forking so our (buffered) output won't
 	 * end up in the test outputs.
 	 */
-
 	fflush(stdout);
 	fflush(stderr);
 
-	if ((child = fork())) {
-		int outfd = outpipe[0];
-		int errfd = errpipe[0];
-		close(outpipe[1]);
-		close(errpipe[1]);
-
-		result = monitor_output(child, outfd, errfd, kmsgfd, sigfd,
-					outputs, time_spent, settings);
-	} else {
-		int outfd = outpipe[1];
-		int errfd = errpipe[1];
+	child = fork();
+	if (child < 0) {
+		fprintf(stderr, "Failed to fork()");
+		result = -1;
+		goto out_kmsgfd;
+	} else if (child == 0) {
+		outfd = outpipe[1];
+		errfd = errpipe[1];
 		close(outpipe[0]);
 		close(errpipe[0]);
 
@@ -974,13 +961,28 @@ static int execute_next_entry(struct execute_state *state,
 		setenv("IGT_SENTINEL_ON_STDERR", "1", 1);
 
 		execute_test_process(outfd, errfd, settings, entry);
+		/* unreachable */
 	}
 
-	/* TODO: Refactor this whole function to use onion teardown */
+	outfd = outpipe[0];
+	errfd = errpipe[0];
 	close(outpipe[1]);
 	close(errpipe[1]);
+	outpipe[1] = errpipe[1] = -1;
+
+	result = monitor_output(child, outfd, errfd, kmsgfd, sigfd,
+				outputs, time_spent, settings);
+
+out_kmsgfd:
 	close(kmsgfd);
+out_pipe:
+	close_outputs(outputs);
+	close(outpipe[0]);
+	close(outpipe[1]);
+	close(errpipe[0]);
+	close(errpipe[1]);
 	close_outputs(outputs);
+out_dirfd:
 	close(dirfd);
 
 	return result;
Signature · 17 lines
2.21.0

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev
#1 lore raw

[igt-dev] ✓ Fi.CI.BAT: success for runner/executor: refactor error handling (rev2)

== Series Details ==

== Series Details ==

Series: runner/executor: refactor error handling (rev2)
URL : https://patchwork.freedesktop.org/series/58235/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5780 -> IGTPW_2666
====================================================

Summary
-------

**SUCCESS**

No regressions found.

External URL: https://patchwork.freedesktop.org/api/1.0/series/58235/revisions/2/mbox/

Known issues
------------

Here are the changes found in IGTPW_2666 that come from known issues:

### IGT changes ###

#### Issues hit ####

* igt@gem_exec_basic@basic-bsd2:
- fi-kbl-7500u: NOTRUN -> SKIP [fdo#109271] +9

* igt@kms_chamelium@dp-crc-fast:
- fi-kbl-7500u: NOTRUN -> DMESG-WARN [fdo#103841]

* igt@runner@aborted:
- fi-kbl-7500u: NOTRUN -> FAIL [fdo#103841]
- fi-apl-guc: NOTRUN -> FAIL [fdo#108622] / [fdo#109720]

#### Possible fixes ####

* igt@i915_selftest@live_uncore:
- fi-ivb-3770: DMESG-FAIL [fdo#110210] -> PASS

{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).

[fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
[fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
[fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
[fdo#109779]: https://bugs.freedesktop.org/show_bug.cgi?id=109779
[fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210

Participating hosts (48 -> 42)
------------------------------

Additional (1): fi-kbl-7500u
Missing (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-blb-e6850

Build changes
-------------

* IGT: IGT_4892 -> IGTPW_2666

CI_DRM_5780: 774f8c588542dda6d73161429cbf1e027789d6ef @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2666: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2666/
IGT_4892: 8ae86621d6fff60b6e20c6b0f9b336785c935b0f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2666/
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Reviewed-by on v2 1/1
#2 lore raw
In reply to Simon Ser
Quoted 6 lines · Simon Ser

* Refactor to use goto error handling
* Make execute_test_process noreturn to remove uninitialized variable warning
* Check fork() return value

and merged with a slight editorial change to the error message in fork() failing. Thanks for the patch.
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev