You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Julian Foad <ju...@btopenworld.com> on 2005/12/12 21:12:10 UTC

Re: New apr_stat/apr_dir_read behavior

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

Re: New apr_stat/apr_dir_read behavior

Posted by Vadim Chekan <ko...@gmail.com>.
On 12/12/05, Julian Foad <ju...@btopenworld.com> wrote:

> When apr_stat() returns APR_INCOMPLETE, presumably finfo->valid tells which
> fields are valid.  This ought to be documented.
>

"finfo" is a return value, if I understand correctly. It is
initialized inside apr_stat(). And as far as I tracked APR,
finfo->valud is not initilized propery, as result it is missing some
"if"s and do nothing. So looks like APR bug to me.
I can give more details when I get home.

> - Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: New apr_stat/apr_dir_read behavior

Posted by "D.J. Heap" <dj...@gmail.com>.
> I just documented the use of APR_INCOMPLETE for both apr_stat and
> apr_dir_read, see revision 356615 in the ASF Subversion repository for
> details.  As far as I can tell the incomplete response just means that
> the finfo isn't complete in both cases, so the response (use the valid
> bitmask to check what part's ok) is the correct solution.
>
> -garrett
>

Thanks...so Subversion needs to be fixed to pay attention to
APR_INCOMPLETE and only use the info that is indicated as valid.

DJ

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: New apr_stat/apr_dir_read behavior

Posted by "D.J. Heap" <dj...@gmail.com>.
> I just documented the use of APR_INCOMPLETE for both apr_stat and
> apr_dir_read, see revision 356615 in the ASF Subversion repository for
> details.  As far as I can tell the incomplete response just means that
> the finfo isn't complete in both cases, so the response (use the valid
> bitmask to check what part's ok) is the correct solution.
>
> -garrett
>

Thanks...so Subversion needs to be fixed to pay attention to
APR_INCOMPLETE and only use the info that is indicated as valid.

DJ

Re: New apr_stat/apr_dir_read behavior

Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Andreas Magnusson wrote:
> 
> First, sorry if I sounded off like I thought it was your (as in APRs) 
> fault of the problem in Subversion. That was not the meaning.

It might well be there is a bug in Subversion.  I do want you to understand
thought that if you are going to hash out changes to APR, it must happen
here, not on the svn dev lists :)

> I also believe that finding a map between user/group/world and ACLs is 
> very difficult, perhaps impossible, so no real blame there!

Yes; this is a non-trivial problem.

> I do *not* think that APR 1.2.2 is in error, but I have to punt the 
> question if the change in apr_open_file() should be proposed for 
> backport to the more seasoned APR developers, as I do not know if the 
> new behavior would need a apr_foo_ex(), or if it would be considered 
> "just a bug fix" (read: what is the expected behavior?).

If Windows is consistent with Unix etc, then it's behavior is 'correct'.
If it is inconsistent, we have a bug.

Bill

Re: New apr_stat/apr_dir_read behavior

Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/20/06, Andreas Magnusson <an...@home.se> wrote:
> Garrett Rooney wrote:
> > On 1/20/06, Andreas Magnusson <an...@home.se> wrote:
> >
> > Well, what I was getting at is that since you can reproduce the
> > problem locally, it would be considerably easier for you to find the
> > change that fixed the problem in the more recent branch (svn log and
> > svn blame should be enough to tell you what revision it was introduced
> > in), and given that, it should be pretty simple for you to apply that
> > change to the 0.9.x source tree and see if it fixes your problem.  If
> > it does, the more experienced developers would have a much easier time
> > reviewing the change and potentially committing it to that branch.
> >
> > -garrett
> >
>
> So true,
> in revision 65343, mturk committed the fix I have tried locally. I have
> not run the tests, but by merging that commit to a checkout of 0.9.7
> makes my little test work the same way as 1.2.2 does. Is this
> information enough, or do you want a patch also? Or more info, tests?
> I'm more than happy to help.

That's enough information, I'll try to take a look at this closer this
weekend once I've got a windows machine in front of me to build on.

-garrett

Re: New apr_stat/apr_dir_read behavior

Posted by Andreas Magnusson <an...@home.se>.
Garrett Rooney wrote:
> On 1/20/06, Andreas Magnusson <an...@home.se> wrote:
> 
> Well, what I was getting at is that since you can reproduce the
> problem locally, it would be considerably easier for you to find the
> change that fixed the problem in the more recent branch (svn log and
> svn blame should be enough to tell you what revision it was introduced
> in), and given that, it should be pretty simple for you to apply that
> change to the 0.9.x source tree and see if it fixes your problem.  If
> it does, the more experienced developers would have a much easier time
> reviewing the change and potentially committing it to that branch.
> 
> -garrett
> 

So true,
in revision 65343, mturk committed the fix I have tried locally. I have 
not run the tests, but by merging that commit to a checkout of 0.9.7 
makes my little test work the same way as 1.2.2 does. Is this 
information enough, or do you want a patch also? Or more info, tests? 
I'm more than happy to help.

/Andreas


Re: New apr_stat/apr_dir_read behavior

Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/20/06, Andreas Magnusson <an...@home.se> wrote:

> I also believe that finding a map between user/group/world and ACLs is
> very difficult, perhaps impossible, so no real blame there!
> I do *not* think that APR 1.2.2 is in error, but I have to punt the
> question if the change in apr_open_file() should be proposed for
> backport to the more seasoned APR developers, as I do not know if the
> new behavior would need a apr_foo_ex(), or if it would be considered
> "just a bug fix" (read: what is the expected behavior?).

Well, what I was getting at is that since you can reproduce the
problem locally, it would be considerably easier for you to find the
change that fixed the problem in the more recent branch (svn log and
svn blame should be enough to tell you what revision it was introduced
in), and given that, it should be pretty simple for you to apply that
change to the 0.9.x source tree and see if it fixes your problem.  If
it does, the more experienced developers would have a much easier time
reviewing the change and potentially committing it to that branch.

-garrett

Re: New apr_stat/apr_dir_read behavior

Posted by Andreas Magnusson <an...@home.se>.
Garrett Rooney wrote:
> On 1/20/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> 
>> If you would like to see 1.2.3 (or 1.3.0) 'corrected' please offer a patch.
>> If you are asking for a backport to 0.9.8-dev, we would consider that as well
>> as long as there's no change to ABI and no change to the 'expected' behavior
>> of the functions.  Or, we have to add an ugly apr_foo_ex() which corresponds
>> to new 1.2 code.
>>
>> We hope someday soon 0.9 goes away, but with so many 2.0 apache and current
>> svn installed users, it will take some time, yet.
> 
> +1, I'd love to see a backport of this fix, especially if it happens
> in time for the next 0.9.x release.  We (where 'we' == subversion
> developers) are probably going to want to do a release of APR in the
> reasonably near future to push out BDB 4.4 support, since Subversion's
> starting to grow support for it now, and in order for that to be
> releasable we'll need a version of APR that has configure glue that
> will find it (which I already committed to APR, it just hasn't made it
> in to a release).
> 
> -garrett
> 

Sorry that I reply to the "wrong" reply, but I'm reading through gmane, 
and I'm not friend with my news reader at the moment.

First, sorry if I sounded off like I thought it was your (as in APRs) 
fault of the problem in Subversion. That was not the meaning.
I was more trying to set the record straight as to where the supposed 
problem came from (in the code).
I also believe that finding a map between user/group/world and ACLs is 
very difficult, perhaps impossible, so no real blame there!
I do *not* think that APR 1.2.2 is in error, but I have to punt the 
question if the change in apr_open_file() should be proposed for 
backport to the more seasoned APR developers, as I do not know if the 
new behavior would need a apr_foo_ex(), or if it would be considered 
"just a bug fix" (read: what is the expected behavior?).

Regards,
/Andreas


Re: New apr_stat/apr_dir_read behavior

Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/20/06, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:

> If you would like to see 1.2.3 (or 1.3.0) 'corrected' please offer a patch.
> If you are asking for a backport to 0.9.8-dev, we would consider that as well
> as long as there's no change to ABI and no change to the 'expected' behavior
> of the functions.  Or, we have to add an ugly apr_foo_ex() which corresponds
> to new 1.2 code.
>
> We hope someday soon 0.9 goes away, but with so many 2.0 apache and current
> svn installed users, it will take some time, yet.

+1, I'd love to see a backport of this fix, especially if it happens
in time for the next 0.9.x release.  We (where 'we' == subversion
developers) are probably going to want to do a release of APR in the
reasonably near future to push out BDB 4.4 support, since Subversion's
starting to grow support for it now, and in order for that to be
releasable we'll need a version of APR that has configure glue that
will find it (which I already committed to APR, it just hasn't made it
in to a release).

-garrett

Re: New apr_stat/apr_dir_read behavior

Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Andreas thanks for a very enlightning analysis...

Andreas Magnusson wrote:
> Why this is done I do not know.

Well, the auto-group Everyone is used to evaluate the unix 'world' ownership
permission.  Sort of crufty emulation, yes, but needed something.

Long term ACL's in a truly generic/crossplatform implementation are needed.

> But I think that 
> is the quest for another discussion, perhaps on the Subversion list.

LOL now why would that be?

Have 1/2 a mind to veto all offlist patches for a while, seeing as there
is no record, no history of how we came to the decisions to change the code.
These are usually the (few and far between) patches that get us in the most
trouble down the road.

Please keep the discussion on an appropriate list.

If you would like to see 1.2.3 (or 1.3.0) 'corrected' please offer a patch.
If you are asking for a backport to 0.9.8-dev, we would consider that as well
as long as there's no change to ABI and no change to the 'expected' behavior
of the functions.  Or, we have to add an ugly apr_foo_ex() which corresponds
to new 1.2 code.

We hope someday soon 0.9 goes away, but with so many 2.0 apache and current
svn installed users, it will take some time, yet.

Bill

Re: New apr_stat/apr_dir_read behavior

Posted by Andreas Magnusson <an...@home.se>.
Garrett Rooney wrote:
> On 12/12/05, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> 
> I just documented the use of APR_INCOMPLETE for both apr_stat and
> apr_dir_read, see revision 356615 in the ASF Subversion repository for
> details.  As far as I can tell the incomplete response just means that
> the finfo isn't complete in both cases, so the response (use the valid
> bitmask to check what part's ok) is the correct solution.
> 
> -garrett
> 

Hi all, I've just completed a look into this problem, and this is my 
conclusion:
In APR 0.9.7 (that is delivered with Subversion 1.3.0 on Windows) there 
is a bug in apr_file_open() in that it ignores the flag APR_READCONTROL. 
This is fixed in 1.2.2. This is the thing that causes the whole problem.

But let us take it from the top:
Subversion calls apr_stat(&finfo, path, APR_FINFO_PROT, pool) to see if 
a file is read-only.
apr_stat() calls resolv_ident() which opens the file to get a handle 
which is passed to apr_file_info_get(). It is here the "bug" occurs. In 
0.9.7 the flag APR_READCONTROL isn't used in apr_open_file() which means 
that we cannot call the Win32 GetSecurityInfo() on the handle (done in 
more_finfo()). This means that we call guess_protection_bits() instead, 
which returns something that is quite, but not entirely correct 
(depending on what you were looking for).
In 1.2.2 instead, we manage to call GetSecurityInfo(), and what is 
returned is the rights the auto-group "Everyone" has on the file. Which 
on my machine are none! Why this is done I do not know. But I think that 
is the quest for another discussion, perhaps on the Subversion list.

/Andreas


Re: New apr_stat/apr_dir_read behavior

Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 12/12/05, Garrett Rooney <ro...@electricjellyfish.net> wrote:

> > 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.
>
> Agreed, I will look into making that change in APR.
>
> > 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?
>
> Will do.

I just documented the use of APR_INCOMPLETE for both apr_stat and
apr_dir_read, see revision 356615 in the ASF Subversion repository for
details.  As far as I can tell the incomplete response just means that
the finfo isn't complete in both cases, so the response (use the valid
bitmask to check what part's ok) is the correct solution.

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: New apr_stat/apr_dir_read behavior

Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 12/12/05, Garrett Rooney <ro...@electricjellyfish.net> wrote:

> > 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.
>
> Agreed, I will look into making that change in APR.
>
> > 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?
>
> Will do.

I just documented the use of APR_INCOMPLETE for both apr_stat and
apr_dir_read, see revision 356615 in the ASF Subversion repository for
details.  As far as I can tell the incomplete response just means that
the finfo isn't complete in both cases, so the response (use the valid
bitmask to check what part's ok) is the correct solution.

-garrett

Re: New apr_stat/apr_dir_read behavior

Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 12/12/05, Julian Foad <ju...@btopenworld.com> wrote:

> Presumably the APR developers have decided that this error code will now have a
> wider meaning than is documented.

See, that's the thing, I'm not sure how this is happening.  The code
in question doesn't seem to have changed in any meaningful way in
quite some time.

> 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.

Agreed, I will look into making that change in APR.

> 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?

Will do.

> 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().)

Yes, it should use APR_STATUS_IS_INCOMPLETE.

-garrett

Re: New apr_stat/apr_dir_read behavior

Posted by "D.J. Heap" <dj...@gmail.com>.
On 12/12/05, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> D.J. Heap wrote:
> >
> > The little testing I did indicated that APR_INCOMPLETE is (in APR
> > 1.2.2) always being returned on Win32 if APR_FINFO_PROT is passed in
> > as 'wanted'.
>
> For stat, lstat, or for getfileinfo?  It's very expensive to compute
> (don't ask if you don't want) but at least stat/lstat by name should
> be able to come up with the 'answer' (hackish as it is.)  Will research.
>
> Bill


apr_stat and apr_dir_read were the functions returning APR_INCOMPLETE in 1.2.2.

Thanks!

DJ

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: New apr_stat/apr_dir_read behavior

Posted by "D.J. Heap" <dj...@gmail.com>.
On 12/12/05, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> D.J. Heap wrote:
> >
> > The little testing I did indicated that APR_INCOMPLETE is (in APR
> > 1.2.2) always being returned on Win32 if APR_FINFO_PROT is passed in
> > as 'wanted'.
>
> For stat, lstat, or for getfileinfo?  It's very expensive to compute
> (don't ask if you don't want) but at least stat/lstat by name should
> be able to come up with the 'answer' (hackish as it is.)  Will research.
>
> Bill


apr_stat and apr_dir_read were the functions returning APR_INCOMPLETE in 1.2.2.

Thanks!

DJ

Re: New apr_stat/apr_dir_read behavior

Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
D.J. Heap wrote:
> 
> The little testing I did indicated that APR_INCOMPLETE is (in APR
> 1.2.2) always being returned on Win32 if APR_FINFO_PROT is passed in
> as 'wanted'.

For stat, lstat, or for getfileinfo?  It's very expensive to compute
(don't ask if you don't want) but at least stat/lstat by name should
be able to come up with the 'answer' (hackish as it is.)  Will research.

Bill

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: New apr_stat/apr_dir_read behavior

Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
D.J. Heap wrote:
> 
> The little testing I did indicated that APR_INCOMPLETE is (in APR
> 1.2.2) always being returned on Win32 if APR_FINFO_PROT is passed in
> as 'wanted'.

For stat, lstat, or for getfileinfo?  It's very expensive to compute
(don't ask if you don't want) but at least stat/lstat by name should
be able to come up with the 'answer' (hackish as it is.)  Will research.

Bill

Re: New apr_stat/apr_dir_read behavior

Posted by "D.J. Heap" <dj...@gmail.com>.
On 12/12/05, Julian Foad <ju...@btopenworld.com> wrote:
[snip]
> 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().
>


Yes, thanks -- you've explained what I was getting at much better.  
By 'handle', I meant that if APR was going to return APR_INCOMPLETE,
then Subversion needs to pay attention and only use the information
that was really available.

The little testing I did indicated that APR_INCOMPLETE is (in APR
1.2.2) always being returned on Win32 if APR_FINFO_PROT is passed in
as 'wanted'.

DJ

Re: New apr_stat/apr_dir_read behavior

Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 12/12/05, Julian Foad <ju...@btopenworld.com> wrote:

> Presumably the APR developers have decided that this error code will now have a
> wider meaning than is documented.

See, that's the thing, I'm not sure how this is happening.  The code
in question doesn't seem to have changed in any meaningful way in
quite some time.

> 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.

Agreed, I will look into making that change in APR.

> 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?

Will do.

> 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().)

Yes, it should use APR_STATUS_IS_INCOMPLETE.

-garrett

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org


Re: New apr_stat/apr_dir_read behavior

Posted by "D.J. Heap" <dj...@gmail.com>.
On 12/12/05, Julian Foad <ju...@btopenworld.com> wrote:
[snip]
> 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().
>


Yes, thanks -- you've explained what I was getting at much better.  
By 'handle', I meant that if APR was going to return APR_INCOMPLETE,
then Subversion needs to pay attention and only use the information
that was really available.

The little testing I did indicated that APR_INCOMPLETE is (in APR
1.2.2) always being returned on Win32 if APR_FINFO_PROT is passed in
as 'wanted'.

DJ

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org