lib/process: Don't abort stdout reads on EINTR

If our read() of the process stdout pipe fails with EINTR (eg, if we
receive a SIGCHLD because the process exited), then
process_read_stdout_once will return a non-zero exit code, and we'll
abort any further stdout collection.

Instead, we should check for EINTR, and allow the reads to continue.

This change normalises the return value from process_read_stdout_once to
return positive on success, negative on failure, and zero on competion.
We use a positive return value for the non-error EINTR case.

Also, add a pb_log if the read fails for non-EINTR reasons.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

diff --git a/lib/process/process.c b/lib/process/process.c
index 86c7fbbf246c374d50f1c23a637118079817a27c..081a48ca36ba27ac37eb1a83888eaec281d635b0 100644 (file)
--- a/lib/process/process.c
+++ b/lib/process/process.c
@@ -61,7 +61,13 @@ static struct process_info *get_info(struct process *process)
 }
 
 /* Read as much as possible into the currently-allocated stdout buffer, and
- * possibly realloc it for the next read */
+ * possibly realloc it for the next read
+ *
+ * Returns:
+ *  > 0 on success (even though no bytes may have been read)
+ *    0 on EOF (no error, but no more reads can be performed)
+ *  < 0 on error
+ **/
 static int process_read_stdout_once(struct process_info *procinfo)
 {
        struct process *process = &procinfo->process;
@@ -74,8 +80,14 @@ static int process_read_stdout_once(struct process_info *procinfo)
        max_len =  procinfo->stdout_buf_len - process->stdout_len - 1;
 
        rc = read(fd, process->stdout_buf + process->stdout_len, max_len);
-       if (rc <= 0)
+       if (rc == 0)
+               return 0;
+       if (rc < 0) {
+               if (errno == EINTR)
+                       return 1;
+               pb_log("%s: read failed: %s\n", __func__, strerror(errno));
                return rc;
+       }
 
        process->stdout_len += rc;
        if (process->stdout_len == procinfo->stdout_buf_len - 1) {
@@ -85,7 +97,7 @@ static int process_read_stdout_once(struct process_info *procinfo)
                                procinfo->stdout_buf_len);
        }
 
-       return rc;
+       return 1;
 }
 
 static int process_setup_stdout_pipe(struct process_info *procinfo)

diff --git a/test/lib/Makefile.am b/test/lib/Makefile.am
index ed570af14605d5610424156d40dc6e947657bc83..23bee364e727f3751e32a786492ac41fe8cdb744 100644 (file)
--- a/test/lib/Makefile.am
+++ b/test/lib/Makefile.am
@@ -32,6 +32,7 @@ check_PROGRAMS = list-test \
        test-process-async-stdout \
        test-process-parent-stdout \
        test-process-both \
+       test-process-stdout-eintr \
        test-fold
 
 TESTS = $(check_PROGRAMS)

diff --git a/test/lib/test-process-stdout-eintr.c b/test/lib/test-process-stdout-eintr.c
new file mode 100644 (file)
index 0000000..b62d570
--- /dev/null
+++ b/test/lib/test-process-stdout-eintr.c
@@ -0,0 +1,57 @@
+
+#include <stdlib.h>
+#include <string.h>
+#include <assert.h>
+#include <signal.h>
+
+#include <process/process.h>
+#include <waiter/waiter.h>
+#include <talloc/talloc.h>
+
+static int do_child(int ppid)
+{
+       sleep(1);
+       kill(ppid, SIGCHLD);
+       printf("forty two\n");
+       return 42;
+}
+
+int main(int argc, char **argv)
+{
+       struct waitset *waitset;
+       struct process *process;
+       const char *child_argv[3];
+       void *ctx;
+
+       if (argc == 3 && !strcmp(argv[1], "child"))
+               return do_child(atoi(argv[2]));
+
+       ctx = talloc_new(NULL);
+
+       waitset = waitset_create(ctx);
+
+       process_init(ctx, waitset, false);
+
+       child_argv[0] = argv[0];
+       child_argv[1] = "child";
+       child_argv[2] = talloc_asprintf(ctx, "%d", getpid());
+       child_argv[3] = NULL;
+
+       process = process_create(ctx);
+       process->path = child_argv[0];
+       process->argv = child_argv;
+       process->keep_stdout = true;
+
+       process_run_sync(process);
+
+       assert(WIFEXITED(process->exit_status));
+       assert(WEXITSTATUS(process->exit_status) == 42);
+
+       assert(process->stdout_len == strlen("forty two\n"));
+       assert(!memcmp(process->stdout_buf, "forty two\n",
+                               process->stdout_len));
+
+       talloc_free(ctx);
+
+       return EXIT_SUCCESS;
+}