From 5353347f23f4a0cb044d1d87d7747dedc71f525c Mon Sep 17 00:00:00 2001 From: Tavian Barnes Date: Tue, 21 Sep 2021 11:56:02 -0400 Subject: 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. --- ctx.c | 72 ++++++++++++++++++++-------------- ctx.h | 13 +++--- parse.c | 21 +++++++++- tests.sh | 55 +++++++++++++++++++++++--- tests/test_fprint_duplicate_stdout.out | 38 ++++++++++++++++++ tests/test_stderr_fails_silently.out | 19 +++++++++ 6 files changed, 177 insertions(+), 41 deletions(-) create mode 100644 tests/test_fprint_duplicate_stdout.out create mode 100644 tests/test_stderr_fails_silently.out 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 -- cgit v1.2.3