As an attempt to make this on-topic for both lists, i won't go
into how i discovered the bug. I don't think it's necessary.
Correctly using waitpid(2) involves checking for EINTR and trying
again. That leads to common usage being something like this:
while (waitpid(pid, &status, 0) < 0) {
if (errno != EINTR) {
break;
}
}
if (status < 0) {
handle error;
} else {
success;
}
It looks like the APR equivalent is supposed to be:
do {
apr_err = apr_proc_wait (&cmd_proc, &exitcode_val,
&exitwhy_val, APR_WAIT);
} while (APR_STATUS_IS_CHILD_NOTDONE (apr_err));
Is that correct? If so, svn_io_run_cmd needs to be fixed to do
that. Currently it only tries once:
/* Wait for the cmd command to finish. */
apr_err = apr_proc_wait (&cmd_proc, &exitcode_val,
&exitwhy_val, APR_WAIT);
if (APR_STATUS_IS_CHILD_NOTDONE (apr_err))
return svn_error_createf
(apr_err, NULL,
"svn_io_run_cmd: error waiting for %s process",
cmd);
Furthermore, apr_proc_wait itself has a problem:
if ((pstatus = waitpid(proc->pid, &exit_int, waitpid_options)) > 0) {
(handle success, i skip this because it is fine)
...
}
else if (pstatus == 0) {
return APR_CHILD_NOTDONE;
}
return errno;
}
That's it. It never handles the case where waitpid(2) returns
-1. But maybe it doesn't need to? Maybe the apr_proc_wait
caller is expected to check for EINTR? But that means the APR
idiom needs to be:
do {
apr_err = apr_proc_wait (&cmd_proc, &exitcode_val,
&exitwhy_val, APR_WAIT);
} while (APR_STATUS_IS_CHILD_NOTDONE (apr_err) || apr_err == EINTR);
Or maybe APR_STATUS_IS_CHILD_NOTDONE should handle that? I don't
know, but *something* needs to change, because currently
svn_io_run_cmd breaks when waitpid(2) is interrupted (which turns
out to be the root of a problem i have been debugging tonight).
It may be as simple as changing APR_STATUS_IS_CHILD_NOTDONE, in
which case apr_proc_wait doesn't need to change at all. But i am
not sure that is the solution. No matter what, svn_io_run_cmd
will need to change so that it repeats the apr_proc_wait call as
necessary (unless you want to make apr_proc_wait itself loop over
waitpid(2), which i think is NOT the way to go).
--
Eric Gillespie, Jr. <*> [EMAIL PROTECTED]
Build a fire for a man, and he'll be warm for a day. Set a man on
fire, and he'll be warm for the rest of his life. -Terry Pratchett
