[Bug] Test 1450.91 Fails on NonStop

[Bug] Test 1450.91 Fails on NonStop

[rsbecker]

Test 1450.91 fails...

I am not sure what is going on here other than test_must_fail is not
processing the completion code.  This was run on both NonStop platform
variants with sh and bash. The expected file contains what is expected.
actual is empty. There are no very large files anywhere in the trash
directory.

expecting success of 1450.91 'fsck error on gitattributes with excessive
size':
        blob=$(test-tool genzeros $((100 * 1024 * 1024 + 1)) | git
hash-object -w --stdin) &&
        test_when_finished "remove_object $blob" &&
        tree=$(printf "100644 blob %s\t%s\n" $blob .gitattributes | git
mktree) &&
        test_when_finished "remove_object $tree" &&
        cat >expected <<-EOF &&
        error in blob $blob: gitattributesLarge: .gitattributes too large to
parse
        EOF
        test_must_fail git fsck --no-dangling >actual 2>&1 &&
        test_cmp expected actual

+ + git hash-object -w --stdin
+ test-tool genzeros 104857601
blob=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
+ test_when_finished remove_object e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
+ + git mktree
+ printf 100644 blob %s\t%s\n e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
.gitattributes
tree=f8614bda784833ac9ab9e82c4ca296948c6ddd60
+ test_when_finished remove_object f8614bda784833ac9ab9e82c4ca296948c6ddd60
+ cat
+ 1> expected 0<<
error in blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391: gitattributesLarge:
.gitattributes too large to parse
+ test_must_fail git fsck --no-dangling
+ 1> actual 2>& 1
test_must_fail: command succeeded: git fsck --no-dangling
error: last command exited with $?=1
not ok 91 - fsck error on gitattributes with excessive size
#
#               blob=$(test-tool genzeros $((100 * 1024 * 1024 + 1)) | git
hash-object -w --stdin) &&
#               test_when_finished "remove_object $blob" &&
#               tree=$(printf "100644 blob %s\t%s\n" $blob .gitattributes |
git mktree) &&
#               test_when_finished "remove_object $tree" &&
#               cat >expected <<-EOF &&
#               error in blob $blob: gitattributesLarge: .gitattributes too
large to parse
#               EOF
#               test_must_fail git fsck --no-dangling >actual 2>&1 &&
#               test_cmp expected actual
#
1..91
J


--
Brief whoami: NonStop&UNIX developer since approximately
UNIX(421664400)
NonStop(211288444200000000)
-- In real life, I talk too much.

RE: [Bug] Test 1450.91 Fails on NonStop

[rsbecker]

On Wednesday, February 15, 2023 12:45 PM, I wrote:
>Test 1450.91 fails...
>
>I am not sure what is going on here other than test_must_fail is not
processing the
>completion code.  This was run on both NonStop platform variants with sh
and bash.
>The expected file contains what is expected.
>actual is empty. There are no very large files anywhere in the trash
directory.
>
>expecting success of 1450.91 'fsck error on gitattributes with excessive
>size':
>        blob=$(test-tool genzeros $((100 * 1024 * 1024 + 1)) | git
hash-object -w --stdin)
>&&
>        test_when_finished "remove_object $blob" &&
>        tree=$(printf "100644 blob %s\t%s\n" $blob .gitattributes | git
>mktree) &&
>        test_when_finished "remove_object $tree" &&
>        cat >expected <<-EOF &&
>        error in blob $blob: gitattributesLarge: .gitattributes too large
to parse
>        EOF
>        test_must_fail git fsck --no-dangling >actual 2>&1 &&
>        test_cmp expected actual
>
>+ + git hash-object -w --stdin
>+ test-tool genzeros 104857601
>blob=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
>+ test_when_finished remove_object
>+ e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
>+ + git mktree
>+ printf 100644 blob %s\t%s\n e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
>.gitattributes
>tree=f8614bda784833ac9ab9e82c4ca296948c6ddd60
>+ test_when_finished remove_object
>+ f8614bda784833ac9ab9e82c4ca296948c6ddd60
>+ cat
>+ 1> expected 0<<
>error in blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391: gitattributesLarge:
>.gitattributes too large to parse
>+ test_must_fail git fsck --no-dangling
>+ 1> actual 2>& 1
>test_must_fail: command succeeded: git fsck --no-dangling
>error: last command exited with $?=1
>not ok 91 - fsck error on gitattributes with excessive size #
>#               blob=$(test-tool genzeros $((100 * 1024 * 1024 + 1)) | git
>hash-object -w --stdin) &&
>#               test_when_finished "remove_object $blob" &&
>#               tree=$(printf "100644 blob %s\t%s\n" $blob .gitattributes |
>git mktree) &&
>#               test_when_finished "remove_object $tree" &&
>#               cat >expected <<-EOF &&
>#               error in blob $blob: gitattributesLarge: .gitattributes too
>large to parse
>#               EOF
>#               test_must_fail git fsck --no-dangling >actual 2>&1 &&
>#               test_cmp expected actual
>#
>1..91
>J

This seems to have started at 2.39. I first noticed it 2.39.2.

Re: [Bug] Test 1450.91 Fails on NonStop

[Jeff King]

On Wed, Feb 15, 2023 at 12:44:59PM -0500, rsbecker@nexbridge.com wrote:

> + + git hash-object -w --stdin
> + test-tool genzeros 104857601
> blob=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391

That's not right. e69de29bb is the empty blob. A hundred megabytes (plus
one) of zeroes should be d748d1b68b9dc69c4717ebc070dbc547d129bcc6.

So that is the root of your problem (the empty file will obviously not
trigger the "too large" fsck check), but I don't know why it would be
failing.

Maybe check the output of "test-tool genzeros 104857601" to make sure
that it as expected? If not, then the bug is somewhere in
t/helper/test-genzeros.c.

It's on the left-hand side of pipe. Maybe it is getting a write() error,
but we are ignoring it? Maybe a patch like this would help diagnose:

diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c
index 8ca988d621..0574917005 100644
--- a/t/helper/test-genzeros.c
+++ b/t/helper/test-genzeros.c
@@ -18,14 +18,14 @@ 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;
+			die_errno("write()");
 
 	while (count > 0) {
 		n = write(1, zeros, count < ARRAY_SIZE(zeros) ?
 			  count : ARRAY_SIZE(zeros));
 
 		if (n < 0)
-			return -1;
+			die_errno("write()");
 
 		count -= n;
 	}

-Peff

RE: [Bug] Test 1450.91 Fails on NonStop

[rsbecker]

On Wednesday, February 15, 2023 1:02 PM, Jeff King wrote:
>On Wed, Feb 15, 2023 at 12:44:59PM -0500, rsbecker@nexbridge.com wrote:
>
>> + + git hash-object -w --stdin
>> + test-tool genzeros 104857601
>> blob=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
>
>That's not right. e69de29bb is the empty blob. A hundred megabytes (plus
>one) of zeroes should be d748d1b68b9dc69c4717ebc070dbc547d129bcc6.
>
>So that is the root of your problem (the empty file will obviously not trigger the "too
>large" fsck check), but I don't know why it would be failing.
>
>Maybe check the output of "test-tool genzeros 104857601" to make sure that it as
>expected? If not, then the bug is somewhere in t/helper/test-genzeros.c.
>
>It's on the left-hand side of pipe. Maybe it is getting a write() error, but we are
>ignoring it? Maybe a patch like this would help diagnose:
>
>diff --git a/t/helper/test-genzeros.c b/t/helper/test-genzeros.c index
>8ca988d621..0574917005 100644
>--- a/t/helper/test-genzeros.c
>+++ b/t/helper/test-genzeros.c
>@@ -18,14 +18,14 @@ 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;
>+			die_errno("write()");
>
> 	while (count > 0) {
> 		n = write(1, zeros, count < ARRAY_SIZE(zeros) ?
> 			  count : ARRAY_SIZE(zeros));
>
> 		if (n < 0)
>-			return -1;
>+			die_errno("write()");
>
> 		count -= n;
> 	}
>

So, this looks like we do not have to hold the 2.39.2 release for this problem. I will run the packaging job for the platform release.

+ test-tool genzeros 104857601
fatal: write(): Invalid function argument

That explains it. We had to push the use of xwrite() because of issues associated with writing large buffers exceeding through write(). We could use MAX_IO_SIZE to limit the write size (at least on NonStop) into acceptable chunks.

Re: [Bug] Test 1450.91 Fails on NonStop

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> So, this looks like we do not have to hold the 2.39.2 release for this problem. I will run the packaging job for the platform release.
>
> + test-tool genzeros 104857601
> fatal: write(): Invalid function argument
>
> That explains it. We had to push the use of xwrite() because of
> issues associated with writing large buffers exceeding through
> write(). We could use MAX_IO_SIZE to limit the write size (at
> least on NonStop) into acceptable chunks.

True.

Curious that this use of write(2) is from late 2021 and its uses in
tests came from the same era (e.g. t1051 has two calls added in the
same timeperiod).  So it is nothing new even to your platform, I
suspect.

There are a handful of raw write() in reftable implementation whose
size I am not sure about (and I am not touching), and a deliberate
use of it in trace2/tr2_dst.c that is well explained (and I do not
think touching is a good idea).  Everything else looks like they are
aware that they are making a short write, but some of them might
still want to become xwrite() to benefit from auto retrying a short
write (which I didn't check).

In any case, I think the attached is a sensible first step to have.

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

diff --git c/t/helper/test-genzeros.c w/t/helper/test-genzeros.c
index 8ca988d621..9a1d3ee221 100644
--- c/t/helper/test-genzeros.c
+++ w/t/helper/test-genzeros.c
@@ -17,12 +17,12 @@ 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)
+		if (xwrite(1, zeros, ARRAY_SIZE(zeros)) < 0)
 			return -1;
 
 	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;

RE: [Bug] Test 1450.91 Fails on NonStop

[rsbecker]

On Wednesday, February 15, 2023 2:10 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> So, this looks like we do not have to hold the 2.39.2 release for this
problem. I will
>run the packaging job for the platform release.
>>
>> + test-tool genzeros 104857601
>> fatal: write(): Invalid function argument
>>
>> That explains it. We had to push the use of xwrite() because of issues
>> associated with writing large buffers exceeding through write(). We
>> could use MAX_IO_SIZE to limit the write size (at least on NonStop)
>> into acceptable chunks.
>
>True.
>
>Curious that this use of write(2) is from late 2021 and its uses in tests
came from the
>same era (e.g. t1051 has two calls added in the same timeperiod).  So it is
nothing
>new even to your platform, I suspect.

I have been looking at the past log (pre-2.39) and this test has not failed
previously - although git blame on t1450 shows 6-Feb-2023, so that might be
why I've not seen this previously. It is possible test-genzeros was not used
in this way previously.

>There are a handful of raw write() in reftable implementation whose size I
am not
>sure about (and I am not touching), and a deliberate use of it in
trace2/tr2_dst.c that
>is well explained (and I do not think touching is a good idea).  Everything
else looks
>like they are aware that they are making a short write, but some of them
might still
>want to become xwrite() to benefit from auto retrying a short write (which
I didn't
>check).

I would not want to touch those either, although I do keep an eye out for
new write() usage and whine about it if I notice. ;-)

>In any case, I think the attached is a sensible first step to have.
>
> t/helper/test-genzeros.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git c/t/helper/test-genzeros.c w/t/helper/test-genzeros.c index
>8ca988d621..9a1d3ee221 100644
>--- c/t/helper/test-genzeros.c
>+++ w/t/helper/test-genzeros.c
>@@ -17,12 +17,12 @@ 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)
>+		if (xwrite(1, zeros, ARRAY_SIZE(zeros)) < 0)
> 			return -1;
>
> 	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;

This works well as a fix. Please go with it with my blessing.
--Randall

Re: [Bug] Test 1450.91 Fails on NonStop

[Jeff King]

On Wed, Feb 15, 2023 at 11:10:14AM -0800, Junio C Hamano wrote:

> > + test-tool genzeros 104857601
> > fatal: write(): Invalid function argument
> >
> > That explains it. We had to push the use of xwrite() because of
> > issues associated with writing large buffers exceeding through
> > write(). We could use MAX_IO_SIZE to limit the write size (at
> > least on NonStop) into acceptable chunks.
> 
> True.
> 
> Curious that this use of write(2) is from late 2021 and its uses in
> tests came from the same era (e.g. t1051 has two calls added in the
> same timeperiod).  So it is nothing new even to your platform, I
> suspect.

I think the latent problem was always there, but almost every call to
"test-tool genzeros" is gated on the EXPENSIVE prerequisite, and the
remainder are quite small. This is the first one that is always run, and
is bigger than the 256k buffer.

(This test actually _is_ kind of slow, so one might argue it should be
marked as EXPENSIVE, too, but obviously that is orthogonal).

> diff --git c/t/helper/test-genzeros.c w/t/helper/test-genzeros.c
> index 8ca988d621..9a1d3ee221 100644
> --- c/t/helper/test-genzeros.c
> +++ w/t/helper/test-genzeros.c
> @@ -17,12 +17,12 @@ 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)
> +		if (xwrite(1, zeros, ARRAY_SIZE(zeros)) < 0)
>  			return -1;

Sheesh. It even occurred to me with the big buffer that write limits
could be the problem, but I thought to myself, "no, we have MAX_IO_SIZE"
for that. The fact that this was write() and not xwrite() totally
escaped my notice. :)

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.

We could also move test-tool away from the left-hand side of the pipe in
the test, which might have made this easier to diagnose. But not
significantly so, and it makes the test even less efficient than it
already is.

-Peff

Re: [Bug] Test 1450.91 Fails on NonStop

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

[PATCH] test-genzeros: avoid raw write(2)

[Junio C Hamano]

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 <rsbecker@nexbridge.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

    Junio C Hamano <gitster@pobox.com> writes:

    > Jeff King <peff@peff.net> 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(-)

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;
 	}
-- 
2.39.2-501-gd9d677b2d8

Re: [PATCH] test-genzeros: avoid raw write(2)

[Jeff King]

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

Re: [PATCH] test-genzeros: avoid raw write(2)

[Junio C Hamano]

Jeff King <peff@peff.net> 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-<.

RE: [PATCH] test-genzeros: avoid raw write(2)

[rsbecker]

On Thursday, February 16, 2023 11:10 AM, Junio C Hamano wrote:
>Jeff King <peff@peff.net> 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.