apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eric Gillespie <...@pretzelnet.org>
Subject Problem with apr_proc_wait and/or svn_io_run_cmd
Date Thu, 23 Jan 2003 04:44:08 GMT
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

Mime
View raw message