D.J. Heap wrote:
> With the new Apache 2.2 and APR 1.2, apr_stat and apr_dir_read now
> return APR_INCOMPLETE which Subversion doesn't handle very well. At
> least, they return it much more often that previous releases did.
(It doesn't really matter but in what way does APR return this "much more often"?)
> This patch gets some things working (I can checkout Subversion and
> create a repository), but I'm not sure if it's safe to ignore
> APR_INCOMPLETE in all these places. Would someone with more APR
> knowledge please take a look?
>
> It looks to me like we need to handle APR_INCOMPLETE almost everywhere
> we call apr_stat or apr_dir_read or any other functions that can
> return APR_INCOMPLETE.
APR_INCOMPLETE is just one failure mode. We should only have to "handle" it in
places where we don't require the APR function to have succeeded, i.e. in cases
where we already have non-default error handling.
To APR people too:
> /** @see APR_STATUS_IS_INCOMPLETE */
> #define APR_INCOMPLETE (APR_OS_START_STATUS + 8)
> /**
> * The character conversion stopped because of an incomplete character or
> * shift sequence at the end of the input buffer.
> * @warning
> * always use this test, as platform-specific variances may meet this
> * more than one error code
> */
> #define APR_STATUS_IS_INCOMPLETE(s) ((s) == APR_INCOMPLETE)
Presumably the APR developers have decided that this error code will now have a
wider meaning than is documented.
Should functions be allowed to return that status without saying so in their
doc strings? I wonder if there's much point in returning it unless callers
know officially that it is possible.
When apr_stat() returns APR_INCOMPLETE, presumably finfo->valid tells which
fields are valid. This ought to be documented.
When apr_dir_read() returns APR_INCOMPLETE, from an API user's point of view it
could mean the list of directory entries is incomplete and/or 'finfo' is
incomplete. How is the user supposed to know? This needs to be documented.
Please could some APR person deal with the above points?
A side-note for Subversion:
Subversion's existing test for APR_INCOMPLETE should be converted to
APR_STATUS_IS_INCOMPLETE. (There is just one, I think: in svn_io_copy_file().)
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 17739)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -1129,7 +1129,7 @@
> only on where read perms are granted. If this fails
> fall through to the apr_file_perms_set() call. */
> status = apr_stat (&finfo, path_apr, APR_FINFO_PROT, pool);
> - if (status)
> + if (status && !APR_STATUS_IS_INCOMPLETE(status))
> {
> if (ignore_enoent && APR_STATUS_IS_ENOENT (status))
> return SVN_NO_ERROR;
(This is in svn_io_set_file_executable().)
No, I think that's the wrong place to add that exception in that function.
Your change causes us to go ahead and use the information that we failed to
obtain. INCOMPLETE should be handled by skipping to the alternate method
(apr_file_attrs_set), like APR_ENOTIMPL is handled: that is, in the "if" four
lines below the one you changed.
> @@ -2533,7 +2533,7 @@
>
> status = apr_stat (&finfo, path_apr, APR_FINFO_PROT, pool);
>
> - if (status)
> + if (status && !APR_STATUS_IS_INCOMPLETE(status))
> return svn_error_wrap_apr (status, _("Can't stat directory '%s'"),
> svn_path_local_style (path, pool));
(This is in dir_make().)
That doesn't look right either. Again, it goes on to use the information that
it failed to get.
It could probably be made to do something appropriate.
> @@ -2615,7 +2615,7 @@
>
> status = apr_dir_read (finfo, wanted, thedir);
>
> - if (status)
> + if (status && !APR_STATUS_IS_INCOMPLETE(status))
> return svn_error_wrap_apr (status, _("Can't read directory"));
>
> if (finfo->fname)
(This is in svn_io_dir_read().)
No, we mustn't ignore the error here. If the Subversion API isn't going to
yield the information requested, it must return an error.
> @@ -2684,7 +2684,7 @@
> apr_err = apr_dir_read (&finfo, wanted, handle);
> if (APR_STATUS_IS_ENOENT (apr_err))
> break;
> - else if (apr_err)
> + else if (apr_err && !APR_STATUS_IS_INCOMPLETE(apr_err))
> {
> return svn_error_wrap_apr
> (apr_err, _("Can't read directory entry in '%s'"),
(This is in svn_io_dir_walk().)
Again, you can't just treat failure as success. I think you need to reconsider
this in the light of whatever decision is made about the meaning of
APR_INCOMPLETE for the function apr_dir_read().
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Dec 12 21:13:49 2005