| Message ID | 08be6d90a4890b63fe9b0885af0df084c9ca81fd.1642691534.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f5f6a6cd47405f32c0217b980cb8ae1c5cce316c |
| Headers | show |
| Series | Reftable coverity fixes | expand |
On Thu, Jan 20 2022, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > If the cleanup fails, there is nothing we can do. > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > --- > reftable/stack_test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/reftable/stack_test.c b/reftable/stack_test.c > index e84f50d27ff..19fe4e20085 100644 > --- a/reftable/stack_test.c > +++ b/reftable/stack_test.c > @@ -90,7 +90,7 @@ static void test_read_file(void) > EXPECT(0 == strcmp(want[i], names[i])); > } > free_names(names); > - remove(fn); > + (void) remove(fn); > } > > static void test_parse_names(void) Well, if we fail here due to a permission error or other I/O weirdness surely it's better to: if (remove(fn) < 0) die_errno("unable to remove '%s'", fn); Otherwise we're just silently sweeping that under the rug, and likely having the "rm -rf" we'll shortly do in test-lib.sh catch it at a distance. Also why are we using remove() here at all? Shouldn't this just be unlink()? Or per the feedback above unlink_or_warn() or remove_or_warn()? I.e. looking at the context we just open()'d this "fn", so we're not unsure if it's a directory or a file, are we?
diff --git a/reftable/stack_test.c b/reftable/stack_test.c index e84f50d27ff..19fe4e20085 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -90,7 +90,7 @@ static void test_read_file(void) EXPECT(0 == strcmp(want[i], names[i])); } free_names(names); - remove(fn); + (void) remove(fn); } static void test_parse_names(void)