Problem with apr_proc_wait and/or svn_io_run_cmd

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. * epg@pretzelnet.org
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org