From: Michael Witten <hidden> Date: 2016-06-15 22:50:56
Currently, building git with:
CFLAGS="-std=c99 -pedantic -Wall -Werror -g -02"
causes gcc 4.5.2 to fail with:
vcs-svn/svndump.c:217:3: error: ISO C forbids 'return' with
expression, in function
returning void
The line in question is this:
return repo_delete(node_ctx.dst);
Because repo_delete returns void (vcs-svn/repo_tree.h:19):
void repo_delete(uint32_t *path);
it would seem like it would be OK, but I guess the
C99 standard is quite particular:
6.8.6.4.1:
A return statement with an expression shall not appear in
a function whose return type is void. A return statement
without an expression shall only appear in a function
whose return type is void.
Signed-off-by: Michael Witten <redacted>
---
vcs-svn/svndump.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:56
Hi,
Michael Witten wrote:
Currently, building git with:
CFLAGS="-std=c99 -pedantic -Wall -Werror -g -02"
causes gcc 4.5.2 to fail with:
vcs-svn/svndump.c:217:3: error: ISO C forbids 'return' with
expression, in function
returning void
[...]
Signed-off-by: Michael Witten <redacted>
Thanks, looks sensible (and thanks to Junio for a pointer). Applied to
git://repo.or.cz/git/jrn.git svn-fe
Next step is to figure out the longstanding mysterious bash + prove
hang in t0081.
Jonathan Nieder (2):
vcs-svn: add missing cast to printf argument
tests: make sure input to sed is newline terminated
Michael Witten (1):
vcs-svn: a void function shouldn't try to return something
t/t9010-svn-fe.sh | 8 ++++++--
vcs-svn/fast_export.c | 3 ++-
vcs-svn/svndump.c | 3 ++-
3 files changed, 10 insertions(+), 4 deletions(-)
The problem is that the sleeps hang around for 100 seconds, and they are
connected to the test script's stdout. It works to run "./t0081-*"
because bash sees the SIGCHLD and knows the script is done. But the
prove program actually ignore the SIGCHLD and waits until stdout and
stderr on the child are closed.
I'm not sure why it hangs with bash and not dash. Perhaps it has to do
with one of them using an internal "sleep" and the other not.
Double-weird is that if you "strace" the prove process, it will still
hang. But if you "strace -f", it _won't_ hang. Which makes no sense,
because the only extra thing happening is strace-ing the now-zombie bash
process and the sleeps which are, well, sleeping.
So there is definitely some deep mystery, but I think the fix can be
simpler: we should not leave processes hanging around that might have
descriptors open. Though the above works, I don't like scripts having to
know about the descriptor 5 magic above. The right solution to me is
either:
1. Close descriptor 5 before running test code. But I'm not sure that
will work, since we actually execute the test code in the _current_
shell, not a subshell. Meaning we would be closing it for good.
It also doesn't address descriptor 4, which might also go to
stdout (if "-v" was used). I think the saving grace is that "prove"
is the only thing that cares, and it doesn't use "-v".
2. Tests should kill their backgrounded sleeps themselves. I think I
saw some "kill $!" lines in there, but maybe we are missing one.
-Peff
And then run the test under prove, and check "ps" for remaining sleep
processes. You will see that the killed process IDs do not match up with
the sleep processes. Except for one sleep process, which actually got
killed. That is the second one in the list above. It works because it's:
{
sleep 100 >input &
} &&
...
kill $!
whereas the other ones are like:
{
do_something &&
sleep 100
} >input &
So my guess is that we have to start a subshell for the latter ones, and
_that_ is what gets killed. And that may explain the bash vs dash
behavior; dash, upon receiving the kill signal, presumably kills its
child, but bash does not (I didn't confirm that; it's just a theory).
I'm not sure what the best solution is. Perhaps putting the subshell
into its own process group and killing the whole PGRP?
-Peff
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:56
Jeff King wrote:
The problem is that the sleeps hang around for 100 seconds, and they are
connected to the test script's stdout. It works to run "./t0081-*"
because bash sees the SIGCHLD and knows the script is done. But the
prove program actually ignore the SIGCHLD and waits until stdout and
stderr on the child are closed.
Strange. Why would prove tell its children to ignore SIGCHLD and
SIGTERM?
Double-weird is that if you "strace" the prove process, it will still
hang. But if you "strace -f", it _won't_ hang.
Well, it hangs for me. :) The strangest aspect is that after 100
seconds, all is well again, which suggests that there's more happening
than an unreaped process.
After lowering the sleep to 15 seconds:
| kill: 19422
| bash(19422)---sleep(19424)
|
| ok 3 - 0-length read, no input available
[...]
| kill: 19431
| bash(19431)---sleep(19434)
| ok 5 - 1-byte read, no input available
[...]
| kill: 19438
| bash(19438)---sleep(19441)
|
| ok 6 - long read (around 8192 bytes)
[...]
| # passed all 13 test(s)
[...]
| 19398 18:31:12 exit_group(0) = ?
| 19397 18:31:12 <... select resumed> ) = ? ERESTARTNOHAND (To be restarted)
| 19397 18:31:12 --- SIGCHLD (Child exited) @ 0 (0) ---
| 19397 18:31:12 select(8, [4 6], NULL, NULL, NULL <unfinished ...>
The test script exits, but "prove" is stuck in select and does not
want to start reaping yet. So presumably the test script's children
are adopted by init. We wait around 13 seconds, and then:
| 19424 18:31:25 <... nanosleep resumed> NULL) = 0
| 19424 18:31:25 close(1) = 0
| 19424 18:31:25 close(2) = 0
| 19424 18:31:25 exit_group(0) = ?
| 19422 18:31:25 <... wait4 resumed> 0x7fff65d1ee6c, 0, NULL) = ? ERESTARTSYS (To be restarted)
| 19422 18:31:25 --- SIGTERM (Terminated) @ 0 (0) ---
The first sleep wakes up and dies. The corresponding subshell
wakes up, reaps the child, and finally accepts SIGTERM.
| 19434 18:31:26 <... nanosleep resumed> NULL) = 0
[...]
| 19438 18:31:26 --- SIGTERM (Terminated) @ 0 (0) ---
Likewise for the second and third sleeps.
| 19397 18:31:26 <... select resumed> ) = 2 (in [4 6])
| 19397 18:31:26 read(4, "", 65536) = 0
| 19397 18:31:26 read(6, "", 65536) = 0
| 19397 18:31:26 wait4(19398, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 19398
Now "prove" wakes up again.
The analagous experiment for dash, mksh, etc shows that their
subshells exec()'d /bin/sleep so the SIGTERM reaches sleep right
away with no trouble.
From: Jeff King <hidden> Date: 2016-06-15 22:50:56
On Tue, Mar 29, 2011 at 06:49:55PM -0500, Jonathan Nieder wrote:
Jeff King wrote:
quoted
The problem is that the sleeps hang around for 100 seconds, and they are
connected to the test script's stdout. It works to run "./t0081-*"
because bash sees the SIGCHLD and knows the script is done. But the
prove program actually ignore the SIGCHLD and waits until stdout and
stderr on the child are closed.
Strange. Why would prove tell its children to ignore SIGCHLD and
SIGTERM?
No, you misunderstand. It is prove itself that ignores the SIGCHLD. It
is stuck in the loop in TAP::Parser::Iterator::Process::_next. It has
gotten SIGCHLD, but it keeps blocking waiting to get EOF on the child's
stdout.
quoted
Double-weird is that if you "strace" the prove process, it will still
hang. But if you "strace -f", it _won't_ hang.
Well, it hangs for me. :) The strangest aspect is that after 100
seconds, all is well again, which suggests that there's more happening
than an unreaped process.
Doesn't that point to an unreaped process? After 100 seconds the sleep
process closes, prove gets EOF, and it completes. Lowering the "100" to
"1" caused a 1-second hang for me.
| 19398 18:31:12 exit_group(0) = ?
| 19397 18:31:12 <... select resumed> ) = ? ERESTARTNOHAND (To be restarted)
| 19397 18:31:12 --- SIGCHLD (Child exited) @ 0 (0) ---
| 19397 18:31:12 select(8, [4 6], NULL, NULL, NULL <unfinished ...>
The test script exits, but "prove" is stuck in select and does not
want to start reaping yet. So presumably the test script's children
are adopted by init. We wait around 13 seconds, and then:
Right, prove is stuck in the select. You can see it even got SIGCHLD
above, and if you check your process list, you will probably see the
defunct bash process. But instead of realizing its child has died, it
insists on waiting until the pipe is closed. Nothing has to be adopted
by init. There are simply still processes with the pipe open.
| 19424 18:31:25 <... nanosleep resumed> NULL) = 0
| 19424 18:31:25 close(1) = 0
| 19424 18:31:25 close(2) = 0
| 19424 18:31:25 exit_group(0) = ?
| 19422 18:31:25 <... wait4 resumed> 0x7fff65d1ee6c, 0, NULL) = ? ERESTARTSYS (To be restarted)
| 19422 18:31:25 --- SIGTERM (Terminated) @ 0 (0) ---
The first sleep wakes up and dies. The corresponding subshell
wakes up, reaps the child, and finally accepts SIGTERM.
Hrm. That's different than what happens on my system. On my system, the
bash process is _already_ dead during the whole procedure, and it is
just the stray sleeps that keep prove waiting.
Maybe different bash versions? Mine is 4.1.5(1) (from debian unstable,
bash_4.1-3).
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:56
(-cc: Michael, to stop spamming a kind bug reporter :))
Jeff King wrote:
On Tue, Mar 29, 2011 at 06:49:55PM -0500, Jonathan Nieder wrote:
quoted
| 19424 18:31:25 <... nanosleep resumed> NULL) = 0
| 19424 18:31:25 close(1) = 0
| 19424 18:31:25 close(2) = 0
| 19424 18:31:25 exit_group(0) = ?
| 19422 18:31:25 <... wait4 resumed> 0x7fff65d1ee6c, 0, NULL) = ? ERESTARTSYS (To be restarted)
| 19422 18:31:25 --- SIGTERM (Terminated) @ 0 (0) ---
The first sleep wakes up and dies. The corresponding subshell
wakes up, reaps the child, and finally accepts SIGTERM.
Hrm. That's different than what happens on my system. On my system, the
bash process is _already_ dead during the whole procedure, and it is
just the stray sleeps that keep prove waiting.
Maybe different bash versions? Mine is 4.1.5(1) (from debian unstable,
bash_4.1-3).
$ dpkg-query -W perl bash
bash 4.1-3
perl 5.10.1-18
Same version here, but I had modified the test a little. *tries the
stock version again* Same behavior still. FWIW I am using the patch
below[1] and invoking the tests as
strace -f -o trace.out prove --exec=bash -v t0081-line-buffer.sh :: -v -i
Did you try my 5>/dev/null patch? With it, I get no hang at all.
Haven't tried it yet but will try.
I really don't like that as a long-term solution. Yes, it gets prove
to stop hanging, but meanwhile we have no control over the child
processes we have spawned. I'd rather just drop the tests.
---
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:56
Jeff King wrote:
No, you misunderstand. It is prove itself that ignores the SIGCHLD. It
is stuck in the loop in TAP::Parser::Iterator::Process::_next. It has
gotten SIGCHLD, but it keeps blocking waiting to get EOF on the child's
stdout.
Thanks for clarifying.
Doesn't that point to an unreaped process? After 100 seconds the sleep
process closes, prove gets EOF, and it completes. Lowering the "100" to
"1" caused a 1-second hang for me.
I guess it's just a matter of terminology. To me, it points to an
undelivered signal, since the problem is not a zombie waiting to be
wait-ed on but a process still alive and sleeping.
But instead of realizing its child has died, it
insists on waiting until the pipe is closed. Nothing has to be adopted
by init. There are simply still processes with the pipe open.
I meant that the sleep process is not a descendant of prove any
more.
| $ ps ax | egrep 'sleep|prove'
| 20203 pts/3 T 0:00 vim utils/prove
| 22654 pts/1 S+ 0:00 /usr/bin/perl /usr/bin/prove --exec=bash
| t0081-line-buffer.sh
| 22688 pts/1 S+ 0:00 sleep 100
| 22694 pts/1 S+ 0:00 sleep 100
| 22727 pts/3 S+ 0:00 egrep sleep|prove
| $ pstree -p 22654
| prove(22654)───bash(22655)
Did you try my 5>/dev/null patch? With it, I get no hang at all.
Now I've tested it and it indeed works as advertised.
Sorry for the lack of clarity.
From: Jeff King <hidden> Date: 2016-06-15 22:50:56
On Tue, Mar 29, 2011 at 07:29:21PM -0500, Jonathan Nieder wrote:
quoted
Did you try my 5>/dev/null patch? With it, I get no hang at all.
Haven't tried it yet but will try.
I really don't like that as a long-term solution. Yes, it gets prove
to stop hanging, but meanwhile we have no control over the child
processes we have spawned. I'd rather just drop the tests.
Agreed, it's a horrible fix. I just wanted to make sure it fixed it for
you, too. After thinking on it more, I figured out an elegant fix.
Patch is below.
-- >8 --
Subject: [PATCH] t0081: kill backgrounded processes more robustly
This test creates several background processes that write to
a fifo and then go to sleep for a while (so the reader of
the fifo does not see EOF).
Each background process is made in a curly-braced block in
the shell, and after we are done reading from the fifo, we
use "kill $!" to kill it off.
For a simple, single-command process, this works reliably
and kills the child sleep process. But for more complex
commands like "make_some_output && sleep", the results are
less predictable. When executing under bash, we end up with
a subshell that gets killed by the $! but leaves the sleep
process still alive.
This is bad not only for process hygeine (we are leaving
random sleep processes to expire after a while), but also
interacts badly with the "prove" command. When prove
executes a test, it does not realize the test is done when
it sees SIGCHLD, but rather waits until the test's stdout
pipe is closed. The orphaned sleep process may keep that
pipe open via test-lib's file descriptor 5, causing prove to
hang for 100 seconds.
The solution is to explicitly use a subshell and to exec the
final sleep process, so that when we "kill $!" we get the
process id of the sleep process.
While we're at it, let's make the "kill" process a little
more robust. Specifically:
1. Wrap the "kill" in a test_when_finished, since we want
to clean up the process whether the test succeeds or not.
2. The "kill" is part of our && chain for test success. It
probably won't fail, but it can if the process has
expired before we manage to kill it. So let's mark it
as OK to fail.
Signed-off-by: Jeff King <redacted>
---
Actually, my (2) above is unlikely to trigger, since the test would have
to hang for 100 seconds first, which probably means it is failing
anyway. But I did run across it when I screwed up my fix.
Also, is it just me, or does it seem weird that test_when_finished
blocks failing can produce test failure? I would think you would be able
to put any old cleanup crap into them and not care whether it worked or
not.
t/t0081-line-buffer.sh | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:56
Jeff King wrote:
While we're at it, let's make the "kill" process a little
more robust. Specifically:
1. Wrap the "kill" in a test_when_finished, since we want
to clean up the process whether the test succeeds or not.
2. The "kill" is part of our && chain for test success. It
probably won't fail, but it can if the process has
expired before we manage to kill it. So let's mark it
as OK to fail.
Signed-off-by: Jeff King <redacted>
---
Actually, my (2) above is unlikely to trigger, since the test would have
to hang for 100 seconds first, which probably means it is failing
anyway. But I did run across it when I screwed up my fix.
That is actually how the tests distinguish between success and
failure. Any idea about a better way?
I was in the process of writing a commit message for the same "exec"
trick, but I'm glad you got to it first. ;-)
Also, is it just me, or does it seem weird that test_when_finished
blocks failing can produce test failure? I would think you would be able
to put any old cleanup crap into them and not care whether it worked or
not.
I'm generally happy that it catches mistakes in cleanup code, but I
could easily be convinced to change it anyway.
The $! gets expanded when this is executed, so later background
processes won't interfere. Good.
Maybe it would be good to factor out a helper to make future
improvements a little easier, like so:
-- 8< --
Subject: t0081: introduce helper to fill a pipe in the background
The fill_input function generates a fifo and runs a command to write
to it and wait a while. The intended use is to test programs that
need to be able to cope with input of limited length without relying
on EOF or SIGPIPE to detect its end.
For example:
fill_input "echo hi" &&
head -1 input
will succeed, while
fill_input "echo hi" &&
head -2 input
will hang waiting for more input.
It works by running the indicated commands followed by
"exec sleep 100" in a background process and registering a clean-up
command to kill it and check if it was killed by SIGPIPE (good) or
ran to completion (bad). This patch adds a definition for this
function and updates existing tests that used that trick to use it.
Signed-off-by: Jonathan Nieder <redacted>
---
The commit message assumes test_when_finished "kill $!", while the
patch uses "kill $! || :"; one of the two would need to be fixed
before applying this.
t/t0081-line-buffer.sh | 51 +++++++++++++++++++----------------------------
1 files changed, 21 insertions(+), 30 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:50:56
On Tue, Mar 29, 2011 at 10:57:33PM -0500, Jonathan Nieder wrote:
Jeff King wrote:
quoted
While we're at it, let's make the "kill" process a little
more robust. Specifically:
1. Wrap the "kill" in a test_when_finished, since we want
to clean up the process whether the test succeeds or not.
2. The "kill" is part of our && chain for test success. It
probably won't fail, but it can if the process has
expired before we manage to kill it. So let's mark it
as OK to fail.
Signed-off-by: Jeff King <redacted>
---
Actually, my (2) above is unlikely to trigger, since the test would have
to hang for 100 seconds first, which probably means it is failing
anyway. But I did run across it when I screwed up my fix.
That is actually how the tests distinguish between success and
failure. Any idea about a better way?
Ah. I was trying not to look too hard at how the tests actually work, so
I completely missed that. Yes, if the "kill" is part of the actual test,
then it should stay in the test. We can also put in a test_when_finished
to kill it should the test fail to make it that far. If the cleanup one
does an extra kill, it shouldn't be a big deal.
I was in the process of writing a commit message for the same "exec"
trick, but I'm glad you got to it first. ;-)
I don't know why I didn't think of it sooner. :)
quoted
Also, is it just me, or does it seem weird that test_when_finished
blocks failing can produce test failure? I would think you would be able
to put any old cleanup crap into them and not care whether it worked or
not.
I'm generally happy that it catches mistakes in cleanup code, but I
could easily be convinced to change it anyway.
I don't think it's a big deal. It just surprised me.
Maybe it would be good to factor out a helper to make future
improvements a little easier, like so:
-- 8< --
Subject: t0081: introduce helper to fill a pipe in the background
The fill_input function generates a fifo and runs a command to write
to it and wait a while. The intended use is to test programs that
need to be able to cope with input of limited length without relying
on EOF or SIGPIPE to detect its end.
Yeah, that looks much nicer. Do you want to just re-roll that on top of
what's in 'master', with the "exec" magic and the defensive
test_when_finished in it (or as a separate patch on top if you want to
split the refactor and fix)? Feel free to borrow from my commit message.
-Peff
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:56
Jeff King wrote:
After thinking on it more, I figured out an elegant fix.
All right, thanks again for this.
I really do want to address your "while at it". I'm pretty
uncomfortable with the "sleep 100" --- it might be better to do "exec
perl -e sleep" so there is no chance that some incredibly slow system
won't make the race. Or a failure mode that does not involve a long
hang would be even better.
Anyway, to make others' life better quickly I've pushed out the easy
part of your fix. It sits with the fixes to other embarrasing bugs.
git://repo.or.cz/git/jrn.git svn-fe
Jeff King (1):
tests: kill backgrounded processes more robustly
Jonathan Nieder (2):
vcs-svn: add missing cast to printf argument
tests: make sure input to sed is newline terminated
Michael Witten (1):
vcs-svn: a void function shouldn't try to return something
t/t0081-line-buffer.sh | 12 ++++++------
t/t9010-svn-fe.sh | 8 ++++++--
vcs-svn/fast_export.c | 3 ++-
vcs-svn/svndump.c | 3 ++-
4 files changed, 16 insertions(+), 10 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:56
The fill_input function generates a fifo and runs a command to write
to it and wait. The intended use is to check that specialized
programs can find the end of their input without reading too much or
relying on EOF or SIGPIPE. For example:
fill_input "echo hi" &&
head -1 input
will succeed, while
fill_input "echo hi" &&
head -2 input
will hang.
It works by running the indicated commands followed by
"exec sleep 100" in a background process; the process ID is later
passed to "kill" to avoid leaving behind random processes.
Several tests already did something like that but this adds some
improvements:
1. Wrap the "kill" in a test_when_finished, since we want
to clean up the process whether the test succeeds or not.
2. Mark the kill as OK to fail. These tests are not about
whether the input generating function dies due to SIGPIPE
or the system is slow enough for the timer to expire early
but about what happens on the downstream end.
3. Mark the relevant tests with the EXECKEEPSPID prerequisite.
Based-on-patch-by: Jeff King [off-list ref]
Improved-by: Johannes Sixt [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
Johannes Sixt wrote:
Note that tests that depend on ( exec ... ) & kill $! must be marked with
the EXECKEEPSPID prerequisite.
Hmm, that's a shame, since the point of writing t0081 was to make sure
some assumptions the line-buffer lib makes are valid on Windows. So I
suspect a better thing to do would be to remove t0081 --- the
line-buffer lib is simple, everyday use exercises it pretty well
already, and I can't imagine this script catching a bug.
t/t0081-line-buffer.sh | 61 ++++++++++++++++++++++--------------------------
1 files changed, 28 insertions(+), 33 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:56
Jeff King wrote:
Ah. I was trying not to look too hard at how the tests actually work, so
I completely missed that. Yes, if the "kill" is part of the actual test,
then it should stay in the test.
Heh, what was the person writing that test thinking? Allowing the
kill to fail would be better --- some future test might have the
generate_lots_of_text part dying of SIGPIPE, which would make later
attempts to kill it fail. Totally cryptic.
What are these tests make to check, anyway?
- "0-length read, no input available" makes some sense. It's checking
that the platform fread can handle requests to read nothing (or that
line-buffer works around it if some esoteric platform cannot).
- "1-byte read, no input available" makes less sense. I just can't
see any reason for it to fail. And it "tests" two functions at
once, for crazy historical reasons.
- "long read" checks that we can read something around the pipe size
and not get totally confused. I can imagine it failing but it
wouldn't be git's fault. It also doesn't seem like a useful test.
I don't think any of them is worth the fuss. So how about this?
-- 8< --
Subject: Revert "t0081 (line-buffer): add buffering tests"
This (morally) reverts commit d280f68313eecb8b3838c70641a246382d5e5343,
which added some tests that are a pain to maintain and are not likely
to find bugs in git.
Helped-by: Jeff King [off-list ref]
Helped-by: Johannes Sixt [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
t/t0081-line-buffer.sh | 106 +-----------------------------------------------
1 files changed, 2 insertions(+), 104 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:56
Jeff King wrote:
That's OK with me. Should it drop test-line-buffer, too?
For now, test-line-buffer is still being used to check the API wrt
early EOF.
In the long term, yes, I would like to replace those tests with tests
of svn-fe itself. The test suite should not be an obstacle to
changing the line-buffer API as long as svn-fe is adjusted correctly
at the same time.
Thanks for looking it over.