ctx: Also deduplicate the standard streams

This fixes some potential missing output when the same file is used in a
redirection and something like -fprint.  The main benefit is smarter
handling of /dev/stdout, which will now share the CFILE* with cout.

6 files changed, 177 insertions, 41 deletions

diff --git a/ctx.c b/ctx.c
index 0b7296c..450d87e 100644
--- a/ctx.c
+++ b/ctx.c
@@ -161,19 +161,10 @@ struct bfs_ctx_file {
 	const char *path;
 };
 
-CFILE *bfs_ctx_open(struct bfs_ctx *ctx, const char *path, bool use_color) {
-	int error = 0;
-
-	CFILE *cfile = cfopen(path, use_color ? ctx->colors : NULL);
-	if (!cfile) {
-		error = errno;
-		goto out;
-	}
-
+struct CFILE *bfs_ctx_dedup(struct bfs_ctx *ctx, CFILE *cfile, const char *path) {
 	struct bfs_stat sb;
 	if (bfs_stat(fileno(cfile->file), NULL, 0, &sb) != 0) {
-		error = errno;
-		goto out_close;
+		return NULL;
 	}
 
 	bfs_file_id id;
@@ -181,37 +172,60 @@ CFILE *bfs_ctx_open(struct bfs_ctx *ctx, const char *path, bool use_color) {
 
 	struct trie_leaf *leaf = trie_insert_mem(&ctx->files, id, sizeof(id));
 	if (!leaf) {
-		error = errno;
-		goto out_close;
+		return NULL;
 	}
 
-	if (leaf->value) {
-		struct bfs_ctx_file *ctx_file = leaf->value;
-		cfclose(cfile);
-		cfile = ctx_file->cfile;
-		goto out;
+	struct bfs_ctx_file *ctx_file = leaf->value;
+	if (ctx_file) {
+		ctx_file->path = path;
+		return ctx_file->cfile;
 	}
 
-	struct bfs_ctx_file *ctx_file = malloc(sizeof(*ctx_file));
+	leaf->value = ctx_file = malloc(sizeof(*ctx_file));
 	if (!ctx_file) {
-		error = errno;
 		trie_remove(&ctx->files, leaf);
-		goto out_close;
+		return NULL;
 	}
 
 	ctx_file->cfile = cfile;
 	ctx_file->path = path;
-	leaf->value = ctx_file;
 	++ctx->nfiles;
+	return cfile;
+}
+
+/** Close a file tracked by the bfs context. */
+static int bfs_ctx_close(struct bfs_ctx *ctx, struct bfs_ctx_file *ctx_file) {
+	CFILE *cfile = ctx_file->cfile;
+
+	if (cfile == ctx->cout) {
+		// Will be checked later
+		return 0;
+	} else if (cfile == ctx->cerr && !ctx_file->path) {
+		// Writes to stderr are allowed to fail silently, unless the same file was used by
+		// -fprint, -fls, etc.
+		return 0;
+	}
 
-	goto out;
+	int ret = 0, error = 0;
+	if (ferror(cfile->file)) {
+		ret = -1;
+		error = EIO;
+	}
+
+	if (cfile == ctx->cerr) {
+		if (fflush(cfile->file) != 0) {
+			ret = -1;
+			error = errno;
+		}
+	} else {
+		if (cfclose(cfile) != 0) {
+			ret = -1;
+			error = errno;
+		}
+	}
 
-out_close:
-	cfclose(cfile);
-	cfile = NULL;
-out:
 	errno = error;
-	return cfile;
+	return ret;
 }
 
 int bfs_ctx_free(struct bfs_ctx *ctx) {
@@ -233,7 +247,7 @@ int bfs_ctx_free(struct bfs_ctx *ctx) {
 		while ((leaf = trie_first_leaf(&ctx->files))) {
 			struct bfs_ctx_file *ctx_file = leaf->value;
 
-			if (cfclose(ctx_file->cfile) != 0) {
+			if (bfs_ctx_close(ctx, ctx_file) != 0) {
 				if (cerr) {
 					bfs_error(ctx, "'%s': %m.\n", ctx_file->path);
 				}
diff --git a/ctx.h b/ctx.h
index f089b84..02d296f 100644
--- a/ctx.h
+++ b/ctx.h
@@ -164,16 +164,19 @@ const struct bfs_groups *bfs_ctx_groups(const struct bfs_ctx *ctx);
 const struct bfs_mtab *bfs_ctx_mtab(const struct bfs_ctx *ctx);
 
 /**
- * Open a file for the bfs context.
+ * Deduplicate an opened file.
  *
  * @param ctx
  *         The bfs context.
- * @param use_color
- *         Whether to use colors if the file is a TTY.
+ * @param cfile
+ *         The opened file.
+ * @param path
+ *         The path to the opened file (or NULL for standard streams).
  * @return
- *         The opened file, or NULL on failure.
+ *         If the same file was opened previously, that file is returned.  If cfile is a new file,
+ *         cfile itself is returned.  If an error occurs, NULL is returned.
  */
-struct CFILE *bfs_ctx_open(struct bfs_ctx *ctx, const char *path, bool use_color);
+struct CFILE *bfs_ctx_dedup(struct bfs_ctx *ctx, struct CFILE *cfile, const char *path);
 
 /**
  * Dump the parsed command line.
diff --git a/parse.c b/parse.c
index 9930c8b..ad8e89c 100644
--- a/parse.c
+++ b/parse.c
@@ -334,12 +334,24 @@ static void init_print_expr(struct parser_state *state, struct expr *expr) {
 static int expr_open(struct parser_state *state, struct expr *expr, const char *path) {
 	struct bfs_ctx *ctx = state->ctx;
 
-	expr->cfile = bfs_ctx_open(ctx, path, state->use_color);
-	if (!expr->cfile) {
+	CFILE *cfile = cfopen(path, state->use_color ? ctx->colors : NULL);
+	if (!cfile) {
 		parse_error(state, "${blu}%s${rs} ${bld}%s${rs}: %m.\n", expr->argv[0], path);
 		return -1;
 	}
 
+	CFILE *dedup = bfs_ctx_dedup(ctx, cfile, path);
+	if (!dedup) {
+		parse_error(state, "${blu}%s${rs} ${bld}%s${rs}: %m.\n", expr->argv[0], path);
+		cfclose(cfile);
+		return -1;
+	}
+
+	expr->cfile = dedup;
+
+	if (dedup != cfile) {
+		cfclose(cfile);
+	}
 	return 0;
 }
 
@@ -3689,6 +3701,11 @@ struct bfs_ctx *bfs_parse_cmdline(int argc, char *argv[]) {
 		goto fail;
 	}
 
+	if (!bfs_ctx_dedup(ctx, ctx->cout, NULL) || !bfs_ctx_dedup(ctx, ctx->cerr, NULL)) {
+		bfs_perror(ctx, "bfs_ctx_dedup()");
+		goto fail;
+	}
+
 	bool stdin_tty = isatty(STDIN_FILENO);
 	bool stdout_tty = isatty(STDOUT_FILENO);
 	bool stderr_tty = isatty(STDERR_FILENO);
diff --git a/tests.sh b/tests.sh
index 0cd978e..0b2d931 100755
--- a/tests.sh
+++ b/tests.sh
@@ -723,6 +723,10 @@ bfs_tests=(
 
     test_execdir_plus
 
+    test_fprint_duplicate_stdout
+    test_fprint_error_stdout
+    test_fprint_error_stderr
+
     test_help
 
     test_hidden
@@ -768,6 +772,10 @@ bfs_tests=(
 
     # PATH_MAX handling
     test_deep_strict
+
+    # Error handling
+    test_stderr_fails_silently
+    test_stderr_fails_loudly
 )
 
 sudo_tests=(
@@ -1683,9 +1691,9 @@ function test_fprint() {
     sort -o scratch/test_fprint.out scratch/test_fprint.out
 
     if [ "$UPDATE" ]; then
-        cp scratch/test_fprint.out "$TESTS/test_fprint.out"
+        cp {scratch,"$TESTS"}/test_fprint.out
     else
-        diff -u scratch/test_fprint.out "$TESTS/test_fprint.out"
+        diff -u {"$TESTS",scratch}/test_fprint.out
     fi
 }
 
@@ -1698,9 +1706,22 @@ function test_fprint_duplicate() {
     sort -o scratch/test_fprint_duplicate.out scratch/test_fprint_duplicate.out
 
     if [ "$UPDATE" ]; then
-        cp scratch/test_fprint_duplicate.out "$TESTS/test_fprint_duplicate.out"
+        cp {scratch,"$TESTS"}/test_fprint_duplicate.out
     else
-        diff -u scratch/test_fprint_duplicate.out "$TESTS/test_fprint_duplicate.out"
+        diff -u {"$TESTS",scratch}/test_fprint_duplicate.out
+    fi
+}
+
+function test_fprint_duplicate_stdout() {
+    touchp scratch/test_fprint_duplicate_stdout.out
+
+    invoke_bfs basic -fprint scratch/test_fprint_duplicate_stdout.out -print >scratch/test_fprint_duplicate_stdout.out
+    sort -o scratch/test_fprint_duplicate_stdout.out{,}
+
+    if [ "$UPDATE" ]; then
+        cp {scratch,"$TESTS"}/test_fprint_duplicate_stdout.out
+    else
+        diff -u {"$TESTS",scratch}/test_fprint_duplicate_stdout.out
     fi
 }
 
@@ -2227,7 +2248,7 @@ function test_fprintf() {
     if [ "$UPDATE" ]; then
         cp scratch/test_fprintf.out "$TESTS/test_fprintf.out"
     else
-        diff -u scratch/test_fprintf.out "$TESTS/test_fprintf.out"
+        diff -u "$TESTS/test_fprintf.out" scratch/test_fprintf.out
     fi
 }
 
@@ -2606,6 +2627,18 @@ function test_fprint_error() {
     fi
 }
 
+function test_fprint_error_stdout() {
+    if [ -e /dev/full ]; then
+        ! quiet invoke_bfs basic -maxdepth 0 -fprint /dev/full >/dev/full
+    fi
+}
+
+function test_fprint_error_stderr() {
+    if [ -e /dev/full ]; then
+        ! invoke_bfs basic -maxdepth 0 -fprint /dev/full 2>/dev/full
+    fi
+}
+
 function test_print0() {
     invoke_bfs basic/a basic/b -print0 >scratch/test_print0.out
 
@@ -3038,6 +3071,18 @@ function test_files0_from_ok() {
     ! printf "basic\0" | quiet invoke_bfs -files0-from - -ok echo {} \;
 }
 
+function test_stderr_fails_silently() {
+    if [ -e /dev/full ]; then
+        bfs_diff -D all basic 2>/dev/full
+    fi
+}
+
+function test_stderr_fails_loudly() {
+    if [ -e /dev/full ]; then
+        ! invoke_bfs -D all basic -false -fprint /dev/full 2>/dev/full
+    fi
+}
+
 
 BOL=
 EOL='\n'
diff --git a/tests/test_fprint_duplicate_stdout.out b/tests/test_fprint_duplicate_stdout.out
new file mode 100644
index 0000000..6c21751
--- /dev/null
+++ b/tests/test_fprint_duplicate_stdout.out
@@ -0,0 +1,38 @@
+basic
+basic
+basic/a
+basic/a
+basic/b
+basic/b
+basic/c
+basic/c
+basic/c/d
+basic/c/d
+basic/e
+basic/e
+basic/e/f
+basic/e/f
+basic/g
+basic/g
+basic/g/h
+basic/g/h
+basic/i
+basic/i
+basic/j
+basic/j
+basic/j/foo
+basic/j/foo
+basic/k
+basic/k
+basic/k/foo
+basic/k/foo
+basic/k/foo/bar
+basic/k/foo/bar
+basic/l
+basic/l
+basic/l/foo
+basic/l/foo
+basic/l/foo/bar
+basic/l/foo/bar
+basic/l/foo/bar/baz
+basic/l/foo/bar/baz
diff --git a/tests/test_stderr_fails_silently.out b/tests/test_stderr_fails_silently.out
new file mode 100644
index 0000000..bb3cd8d
--- /dev/null
+++ b/tests/test_stderr_fails_silently.out
@@ -0,0 +1,19 @@
+basic
+basic/a
+basic/b
+basic/c
+basic/e
+basic/g
+basic/i
+basic/j
+basic/k
+basic/l
+basic/c/d
+basic/e/f
+basic/g/h
+basic/j/foo
+basic/k/foo
+basic/l/foo
+basic/k/foo/bar
+basic/l/foo/bar
+basic/l/foo/bar/baz