<div>On Wed, Mar 20, 2019 at 8:34 PM Ser, Simon <<a href="mailto:simon.ser@intel.com">simon.ser@intel.com</a>> wrote:<br></div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">* Refactor to use goto error handling<br>
* Make execute_test_process noreturn to remove uninitialized variable<br>
  warning<br>
* Check fork() return value<br>
<br>
Signed-off-by: Simon Ser <<a href="mailto:simon.ser@intel.com" target="_blank">simon.ser@intel.com</a>><br>
---<br>
 runner/executor.c | 63 ++++++++++++++++++++++++-----------------------<br>
 1 file changed, 32 insertions(+), 31 deletions(-)<br>
<br>
diff --git a/runner/executor.c b/runner/executor.c<br>
index d535c276..6bf7ce1c 100644<br>
--- a/runner/executor.c<br>
+++ b/runner/executor.c<br>
@@ -753,9 +753,10 @@ static int monitor_output(pid_t child,<br>
        return killed;<br>
 }<br>
<br>
-static void execute_test_process(int outfd, int errfd,<br>
-                                struct settings *settings,<br>
-                                struct job_list_entry *entry)<br>
+static void __attribute__((noreturn))<br>
+execute_test_process(int outfd, int errfd,<br>
+                    struct settings *settings,<br>
+                    struct job_list_entry *entry)<br>
 {<br>
        char *argv[4] = {};<br>
        size_t rootlen;<br>
@@ -884,9 +885,9 @@ static int execute_next_entry(struct execute_state<br>
*state,<br>
        }<br>
<br>
        if (!open_output_files(dirfd, outputs, true)) {<br>
-               close(dirfd);<br>
                fprintf(stderr, "Error opening output files\n");<br>
-               return -1;<br>
+               result = -1;<br>
+               goto out_dirfd;<br>
        }<br>
<br>
        if (settings->sync) {<br>
@@ -895,14 +896,9 @@ static int execute_next_entry(struct execute_state<br>
*state,<br>
        }<br>
<br>
        if (pipe(outpipe) || pipe(errpipe)) {<br>
-               close_outputs(outputs);<br>
-               close(dirfd);<br>
-               close(outpipe[0]);<br>
-               close(outpipe[1]);<br>
-               close(errpipe[0]);<br>
-               close(errpipe[1]);<br>
                fprintf(stderr, "Error creating pipes: %s\n",<br>
strerror(errno));<br>
-               return -1;<br>
+               result = -1;<br>
+               goto out_pipe;<br>
        }<br>
<br>
        if ((kmsgfd = open("/dev/kmsg", O_RDONLY | O_CLOEXEC)) < 0) {<br>
@@ -923,14 +919,8 @@ static int execute_next_entry(struct execute_state<br>
*state,<br>
        if (sigfd < 0) {<br>
                /* TODO: Handle better */<br>
                fprintf(stderr, "Cannot monitor child process with<br>
signalfd\n");<br>
-               close(outpipe[0]);<br>
-               close(errpipe[0]);<br>
-               close(outpipe[1]);<br>
-               close(errpipe[1]);<br>
-               close(kmsgfd);<br>
-               close_outputs(outputs);<br>
-               close(dirfd);<br>
-               return -1;<br>
+               result = -1;<br>
+               goto out_kmsgfd;<br>
        }<br>
<br>
        if (settings->log_level >= LOG_LEVEL_NORMAL) {<br>
@@ -951,19 +941,15 @@ static int execute_next_entry(struct<br>
execute_state *state,<br>
         * Flush outputs before forking so our (buffered) output won't<br>
         * end up in the test outputs.<br>
         */<br>
-<br>
        fflush(stdout);<br>
        fflush(stderr);<br>
<br>
-       if ((child = fork())) {<br>
-               int outfd = outpipe[0];<br>
-               int errfd = errpipe[0];<br>
-               close(outpipe[1]);<br>
-               close(errpipe[1]);<br>
-<br>
-               result = monitor_output(child, outfd, errfd, kmsgfd,<br>
sigfd,<br>
-                                       outputs, time_spent, settings);<br>
-       } else {<br>
+       child = fork();<br>
+       if (child < 0) {<br>
+               fprintf(stderr, "Failed to fork()");<br>
+               result = -1;<br>
+               goto out_kmsgfd;<br>
+       } else if (child == 0) {<br>
                int outfd = outpipe[1];<br>
                int errfd = errpipe[1];<br>
                close(outpipe[0]);<br>
@@ -974,13 +960,28 @@ static int execute_next_entry(struct<br>
execute_state *state,<br>
                setenv("IGT_SENTINEL_ON_STDERR", "1", 1);<br>
<br>
                execute_test_process(outfd, errfd, settings, entry);<br>
+               /* unreachable */<br>
        }<br>
<br>
-       /* TODO: Refactor this whole function to use onion teardown */<br>
+       int outfd = outpipe[0];<br>
+       int errfd = errpipe[0];<br>
        close(outpipe[1]);<br>
        close(errpipe[1]);<br>
+       outpipe[1] = errpipe[1] = -1;<br>
+<br>
+       result = monitor_output(child, outfd, errfd, kmsgfd, sigfd,<br>
+                               outputs, time_spent, settings);<br>
+<br>
+out_kmsgfd:<br>
        close(kmsgfd);<br>
+out_pipe:<br>
+       close_outputs(outputs);<br>
+       close(outpipe[0]);<br>
+       close(outpipe[1]);<br>
+       close(errpipe[0]);<br>
+       close(errpipe[1]);<br>
        close_outputs(outputs);<br>
+out_dirfd:<br>
        close(dirfd);<br>
<br>
        return result;<br>
-- <br>
2.21.0<br>
<br>
---------------------------------------------------------------------<br>
Intel Finland Oy<br>
Registered Address: PL 281, 00181 Helsinki <br>
Business Identity Code: 0357606 - 4 <br>
Domiciled in Helsinki <br>
<br>
This e-mail and any attachments may contain confidential material for<br>
the sole use of the intended recipient(s). Any review or distribution<br>
by others is strictly prohibited. If you are not the intended<br>
recipient, please contact the sender and delete all copies.</blockquote><div dir="auto"><br></div><div dir="auto">Hello Simon,</div><div dir="auto"><br></div><div dir="auto">Welcome to igt.</div><div dir="auto"><br></div><div dir="auto">The above corporate boilerplate text should be removed from emails to public mailing lists, and definitely from emails with code that you are distributing under the MIT License.</div><div dir="auto"><br></div><div dir="auto">I expect that you didn’t really intend for that paragraph to apply to your patch - but in particular the restrictions on distribution by others is problematic.</div><div dir="auto"><br></div><div dir="auto"><div><a href="https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/CONTRIBUTING.md">https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/CONTRIBUTING.md</a></div><br></div><div dir="auto">You can read further there on the Developer’s Certificate of Origin that igt adopts and what it entails, which I understand is what you intended to use instead of this boilerplate.</div><div dir="auto"><br></div><div dir="auto">Other Intel contributors might be able to assist you with whatever internal corporate proxy changes you need to make.</div><div dir="auto"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
_______________________________________________<br>
igt-dev mailing list<br>
<a href="mailto:igt-dev@lists.freedesktop.org" target="_blank">igt-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/igt-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/igt-dev</a></blockquote></div></div>