test-genzeros: avoid raw write(2)

Commit Message

Junio C Hamano Feb. 16, 2023, 2:56 a.m. UTC

This test helper feeds 256kB of data at once to a single invocation
of the write(2) system call, which may be too much for some
platforms.

Call our xwrite() wrapper that knows to honor MAX_IO_SIZE limit and
cope with short writes due to EINTR instead, and die a bit more
loudly by calling die_errno() when xwrite() indicates an error.

Reported-by: Randall S. Becker <[e-mail]>
Helped-by: Jeff King <[e-mail]>
Signed-off-by: Junio C Hamano <[e-mail]>
---

    Junio C Hamano <[e-mail]> writes:

    > Jeff King <[e-mail]> writes:
    >
    >> So yeah, this seems like an obvious good fix. Using write_or_die() might
    >> be even better, as it would report errors rather than quietly returning
    >> non-zero.
    >
    > Maybe.  Let me cook up a version with a proposed log message.

    I ended up avoiding write_or_die() primarily because there is
    nothing "Git" about this helper, which is a poor-man's emulation
    of "dd if=/dev/zero".  It felt a bit too much to pull cache.h in
    as dependency for it.

 t/helper/test-genzeros.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Jeff King Feb. 16, 2023, 4:34 a.m. UTC | #1

On Wed, Feb 15, 2023 at 06:56:14PM -0800, Junio C Hamano wrote:

> This test helper feeds 256kB of data at once to a single invocation
> of the write(2) system call, which may be too much for some
> platforms.
> 
> Call our xwrite() wrapper that knows to honor MAX_IO_SIZE limit and
> cope with short writes due to EINTR instead, and die a bit more
> loudly by calling die_errno() when xwrite() indicates an error.

Thanks, this looks good to me.

>     I ended up avoiding write_or_die() primarily because there is
>     nothing "Git" about this helper, which is a poor-man's emulation
>     of "dd if=/dev/zero".  It felt a bit too much to pull cache.h in
>     as dependency for it.

I don't find it any more "Git" than xwrite() or die_errno(), really, but
I am quite happy with what you have here.

-Peff

Junio C Hamano Feb. 16, 2023, 4:09 p.m. UTC | #2

Jeff King <[e-mail]> writes:

>>     I ended up avoiding write_or_die() primarily because there is
>>     nothing "Git" about this helper, which is a poor-man's emulation
>>     of "dd if=/dev/zero".  It felt a bit too much to pull cache.h in
>>     as dependency for it.
>
> I don't find it any more "Git" than xwrite() or die_errno(), really, but
> I am quite happy with what you have here.

True.  I view "git-compat-util.h" as "projects, not limited to Git,
would benefit by its help to use POSIX api more sensibly", while
"cache.h" is "things that are about Git".  write_or_die() is certainly
in the former category, but is not available without "cache.h" X-<.

Randall S. Becker Feb. 16, 2023, 4:14 p.m. UTC | #3

On Thursday, February 16, 2023 11:10 AM, Junio C Hamano wrote:
>Jeff King <[e-mail]> writes:
>
>>>     I ended up avoiding write_or_die() primarily because there is
>>>     nothing "Git" about this helper, which is a poor-man's emulation
>>>     of "dd if=/dev/zero".  It felt a bit too much to pull cache.h in
>>>     as dependency for it.
>>
>> I don't find it any more "Git" than xwrite() or die_errno(), really,
>> but I am quite happy with what you have here.
>
>True.  I view "git-compat-util.h" as "projects, not limited to Git, would
benefit by its
>help to use POSIX api more sensibly", while "cache.h" is "things that are
about Git".
>write_or_die() is certainly in the former category, but is not available
without
>"cache.h" X-<.

I remember by first time in there, and thought why can't we use gnulib or
libc and then realized that those dependencies don't port well either (no
gcc on box). I am grateful for the existence of git-compat-util.

Patch

diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c
index 8ca988d621..47af843b68 100644
--- a/t/helper/test-genzeros.c
+++ b/t/helper/test-genzeros.c
@@ -17,15 +17,16 @@  int cmd__genzeros(int argc, const char **argv)
 
 	/* Writing out individual NUL bytes is slow... */
 	while (count < 0)
-		if (write(1, zeros, ARRAY_SIZE(zeros)) < 0)
-			return -1;
+		if (xwrite(1, zeros, ARRAY_SIZE(zeros)) < 0)
+			die_errno("write error");
 
 	while (count > 0) {
-		n = write(1, zeros, count < ARRAY_SIZE(zeros) ?
-			  count : ARRAY_SIZE(zeros));
+		n = xwrite(1, zeros,
+			   count < ARRAY_SIZE(zeros)
+			   ? count : ARRAY_SIZE(zeros));
 
 		if (n < 0)
-			return -1;
+			die_errno("write error");
 
 		count -= n;
 	}