| Message ID | 20231012160930.330618-2-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prevent re-reading 4 GiB files on every status | expand |
On Thu, Oct 12, 2023 at 12:10 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > In a future commit, we're going to work with some large files which will > be at least 4 GiB in size. To take advantage of the sparseness > functionality on most Unix systems and avoid running the system out of > disk, it would be convenient to use truncate(2) to simply create a > sparse file of sufficient size. > > However, the GNU truncate(1) utility isn't portable, so let's write a > tiny test helper that does the work for us. > > Signed-off-by: brian m. carlson <bk2204@github.com> > --- > diff --git a/t/helper/test-truncate.c b/t/helper/test-truncate.c > @@ -0,0 +1,27 @@ > +int cmd__truncate(int argc, const char **argv) > +{ > + char *p = NULL; > + uintmax_t sz = 0; > + int fd = -1; > + > + if (argc != 3) > + die("expected filename and size"); > + > + sz = strtoumax(argv[2], &p, 0); > + if (*p) > + die("invalid size"); Do you want to check 'errno' here, as well (probably before the '*p' check)? Or is that being too defensive for a 'test-tool' command? > + fd = open(argv[1], O_WRONLY | O_CREAT, 0600); > + if (fd < 0) > + die_errno("failed to open file %s", argv[1]); > + > + if (ftruncate(fd, (off_t) sz) < 0) > + die_errno("failed to truncate file"); > + return 0; > +}
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > + fd = open(argv[1], O_WRONLY | O_CREAT, 0600); > + if (fd < 0) > + die_errno("failed to open file %s", argv[1]); contrib/coccinelle/xopen.cocci tells us to write this simply as fd = xopen(argv[1], O_WRONLY | O_CREAT, 0600); https://github.com/git/git/actions/runs/6500777388/job/17656837393#step:4:657
On 2023-10-12 at 22:52:59, Junio C Hamano wrote: > "brian m. carlson" <sandals@crustytoothpaste.net> writes: > > > + fd = open(argv[1], O_WRONLY | O_CREAT, 0600); > > + if (fd < 0) > > + die_errno("failed to open file %s", argv[1]); > > contrib/coccinelle/xopen.cocci tells us to write this simply as > > fd = xopen(argv[1], O_WRONLY | O_CREAT, 0600); Sure, I can do that.
On 2023-10-12 at 17:49:13, Eric Sunshine wrote: > > + sz = strtoumax(argv[2], &p, 0); > > + if (*p) > > + die("invalid size"); > > Do you want to check 'errno' here, as well (probably before the '*p' check)? > > Or is that being too defensive for a 'test-tool' command? I don't believe that's necessary. The Linux manual page leads me to believe that if *argv[2] is not 0 but *p is 0, then the entire string is valid, which would imply that errno is not set. I'm happy to ignore for the moment the case where the user specifies "" as the argument, because it is a test helper and "don't do weird, unexpected things with the test helper without looking at the source code first" is legitimate advice for Git developers. If this were user-facing, improved robustness would be warranted, I agree.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On 2023-10-12 at 22:52:59, Junio C Hamano wrote: >> "brian m. carlson" <sandals@crustytoothpaste.net> writes: >> >> > + fd = open(argv[1], O_WRONLY | O_CREAT, 0600); >> > + if (fd < 0) >> > + die_errno("failed to open file %s", argv[1]); >> >> contrib/coccinelle/xopen.cocci tells us to write this simply as >> >> fd = xopen(argv[1], O_WRONLY | O_CREAT, 0600); > > Sure, I can do that. Unless there are other changes needed, I'll handle it on this end. Thanks.
On Thu, Oct 12, 2023 at 04:09:29PM +0000, brian m. carlson wrote: > +int cmd__truncate(int argc, const char **argv) > +{ > + char *p = NULL; > + uintmax_t sz = 0; > + int fd = -1; > + > + if (argc != 3) > + die("expected filename and size"); > + > + sz = strtoumax(argv[2], &p, 0); > + if (*p) > + die("invalid size"); > + > + fd = open(argv[1], O_WRONLY | O_CREAT, 0600); > + if (fd < 0) > + die_errno("failed to open file %s", argv[1]); > + > + if (ftruncate(fd, (off_t) sz) < 0) > + die_errno("failed to truncate file"); > + return 0; > +} Coverity flagged this as leaking the descriptor "fd" (which is obviously true). I guess it doesn't matter much in practice, since we're exiting the process directly afterwards. If it were a memory leak we'd free() it anyway to shut up the leak detector, but I don't know if we want to be as careful here (in theory it creates noise that distracts from real problems, but Coverity is noisy enough that I don't even bother looking at existing problems). -Peff
diff --git a/Makefile b/Makefile index 9c6a2f125f..03adcb5a48 100644 --- a/Makefile +++ b/Makefile @@ -852,6 +852,7 @@ TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o TEST_BUILTINS_OBJS += test-submodule.o TEST_BUILTINS_OBJS += test-subprocess.o TEST_BUILTINS_OBJS += test-trace2.o +TEST_BUILTINS_OBJS += test-truncate.o TEST_BUILTINS_OBJS += test-urlmatch-normalization.o TEST_BUILTINS_OBJS += test-userdiff.o TEST_BUILTINS_OBJS += test-wildmatch.o diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 9010ac6de7..876cd2dc31 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -86,6 +86,7 @@ static struct test_cmd cmds[] = { { "submodule-nested-repo-config", cmd__submodule_nested_repo_config }, { "subprocess", cmd__subprocess }, { "trace2", cmd__trace2 }, + { "truncate", cmd__truncate }, { "userdiff", cmd__userdiff }, { "urlmatch-normalization", cmd__urlmatch_normalization }, { "xml-encode", cmd__xml_encode }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index f134f96b97..70dd4eba11 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -79,6 +79,7 @@ int cmd__submodule_config(int argc, const char **argv); int cmd__submodule_nested_repo_config(int argc, const char **argv); int cmd__subprocess(int argc, const char **argv); int cmd__trace2(int argc, const char **argv); +int cmd__truncate(int argc, const char **argv); int cmd__userdiff(int argc, const char **argv); int cmd__urlmatch_normalization(int argc, const char **argv); int cmd__xml_encode(int argc, const char **argv); diff --git a/t/helper/test-truncate.c b/t/helper/test-truncate.c new file mode 100644 index 0000000000..bd3fde364b --- /dev/null +++ b/t/helper/test-truncate.c @@ -0,0 +1,27 @@ +#include "test-tool.h" +#include "git-compat-util.h" + +/* + * Truncate a file to the given size. + */ +int cmd__truncate(int argc, const char **argv) +{ + char *p = NULL; + uintmax_t sz = 0; + int fd = -1; + + if (argc != 3) + die("expected filename and size"); + + sz = strtoumax(argv[2], &p, 0); + if (*p) + die("invalid size"); + + fd = open(argv[1], O_WRONLY | O_CREAT, 0600); + if (fd < 0) + die_errno("failed to open file %s", argv[1]); + + if (ftruncate(fd, (off_t) sz) < 0) + die_errno("failed to truncate file"); + return 0; +}