On Tue, Jan 08, 2002 at 06:44:46PM -0500, Garrett Rooney wrote:
> On Tue, Jan 08, 2002 at 01:06:08PM -0600, Karl Fogel wrote:
>
> > Ooooh, that's bad. Suspect something wrong with the error checking in
> > this part of libsvn_wc/get_editor.c, but not sure. Let us know what
> > you find, thanks for looking into this...
> >
> > [...]
> > apr_err = apr_proc_create (&diff_proc,
> > SVN_CLIENT_DIFF,
> > diff_args,
> > NULL,
> > diffproc_attr,
> > fb->pool);
> > if (! APR_STATUS_IS_SUCCESS (apr_err))
> > return svn_error_createf
> > (apr_err, 0, NULL, fb->pool,
> > "close_file: error starting diff process");
> >
> > /* Wait for the diff command to finish. */
> > apr_err = apr_proc_wait (&diff_proc, NULL, NULL, APR_WAIT);
> > if (APR_STATUS_IS_CHILD_NOTDONE (apr_err))
> > return svn_error_createf
> > (apr_err, 0, NULL, fb->pool,
> > "close_file: error waiting for diff process");
> > [...]
>
> ok, the problem is in this code, but the version of it in
> svn_io_run_cmd (for this case, although i imagine it will fail in the
> same manner elsewhere).
>
> when we run apr_proc_create, it forks off a process, which suceeds,
> so that suceeds just fine. that process tries to exec diff, which
> fails for obvious reasons. it then does an 'exit(-1)'. (this occurs
> in apr/threadproc/unix/proc.c for those of you playing along at home)
> when we do the apr_proc_wait, the APR_STATUS_OS_CHILD_NOTDONE will
> return false, because the child is done, it exited with a -1. now we
> don't always call apr_proc_wait with an exitcode parameter, but if we
> did (as we do in svn_io_run_cmd, but not in get_editor.c), we would
> recieve from apr the output of this line:
>
> *exitcode = WEXITSTATUS(exit_int);
>
> which turns out to be 255, which i imagine it some kind of error code
> for this sort of thing.
WEXITSTATUS will return the least significant byte of the error as an int.
Therefor 0xFFFFFFFFFFFF gets truncated to 0xFFFF which is the 255. As far as
I know there is know way for apr to be able to portably be able to tell that
an executable didn't run (other than to stat it first).
Just be happy there is a return code. I had to fight for it :)
>
> now the question is what to do about it. i have a patch that will
> just handle this for SVN_CLIENT_DIFF. we can just check that diff
> exited with either 0, 1, or 2 (the only exit codes it is supposed to
> use, according to it's man page), and if it exits with something else,
> we know there's a problem and we error out.
>
+1 for this approach. We should also be doing something similar when we run
patch. In fact I beleive there are/were comments to the effect of
/* ### need to do better handling of the exit status diff left us here. */
>
> that seems kind of ugly though, and it seems like there must be some
> kind of facility in apr for checking for this condition, or if there
> isn't, there should be.
>
> unfortunately, i can't find it, and if it isn't there now i'm not sure
> how one should implement it.
>
Don't think there is one, and I'm not sure how it would be implemented other
than stating to make sure the file is there and checking
finfo.protection & APR_[UGO]EXECUTE.
>
> in any case, i will be following an issue about this, as soon as i take
> a break and eat some dinner.
>
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- application/pgp-signature attachment: stored
Received on Sat Oct 21 14:36:55 2006