apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <...@apache.org>
Subject Re: cvs commit: apr/test testfile.c
Date Sun, 29 Dec 2002 19:03:44 GMT

On Sun, 29 Dec 2002, William A. Rowe, Jr. wrote:

> At 12:40 AM 12/29/2002, rbb@apache.org wrote:
> >-1.
> >
> >On 29 Dec 2002 wrowe@apache.org wrote:
> >
> >> wrowe       2002/12/28 21:48:51
> >>
> >>   Modified:    test     testfile.c
> >>   Log:
> >>     Remove a bogus test.  All of the original architects have argued against
> >>     testing for bogus (invalid) combinations of bits.  This test checked a
> >>     POSIX behavior of requiring the caller to request read and/or write access.
> >>
> >>     On Win32 and some other platforms that support metadata access, opening
> >>     a file internally for no read or write access is entirely valid.  E.g. to
> >>     modify the meta information, especially about directories on platforms
> >>     that don't support opening 'directories' for read/write access, APR already
> >>     uses apr_file_open() to handle the file metadata access.  I see no
> >>     compelling reason to emulate POSIX's fail-on-no-read-or-write-request
> >>     behavior.
> >
> >The compelling reason is consistancy, which is the whole point of APR!
> >Continue to use apr_file_open internally for whatever you want, but the
> >API that you present to the user _must_ be consistant in how it deals with
> >errors for ALL platforms.  That must be one of the major tenants of APR,
> >or the project is completely useless.
> Ok... I'm baffled...
> you are [or were?] one of the prime supporters of "No Argument Validation"
> for API calls.  We are *not* talking about external error events here, we are
> discussing *developer errors*.  If the developer doesn't open the file for
> APR_READ and it fails the next call to apr_file_read(), you expect they
> will truly come crying at us?

No Will, we are talking about an external API that behaves differently on
different platforms.  That is the problem that APR is trying to solve.
This change makes APR less portable, just like the change to the testdso
test.  If a Windows programmer decides to open the file for neither READ
nor WRITE, that will work on Windows, but it will fail on _most_ Unix

As for no argument validation, that rule comes from the old httpd days,
and it does make sense.  It doesn't make sense to do input validation to
ensure that we don't segfault because a programmer passed in NULL when we
needed a valid pointer.  That mistake is guaranteed to be caught the first
time they run the program in their testing.  It should be fixed quickly.
The goal of no input validation is to provide feedback as quickly as
possible.  There are multiple layers:

1)  Compile time errors/warnings.  -- These are the highest level and
tell a programmer that they have a problem using the API.  These problems
are syntactic problems

2)  Run-time segfault  -- This is the middle of the road.  Still says that
there is a problem, but this time the problem is semantic.  The goal is to
fail horribly so that the programmer can't ignore the mistake.

3)  Run-time error result  -- This is the worst kind of feedback.
Again, this is a semantic problem, but the mis-understanding isn't as
bad as the last one.  Most programmers forget to check error codes for
simple functions, which means that this type of error often seeps through
the cracks.

The rule about input validation is to try to push errors that _could_ have
been type 3 to type 2.  What we are talking about here is a type 3 error
that can't be pushed to type 2.  What I am asking for is consistancy in
how the function behaves so that when a programmer makes a mistake on
Windows it fails the same way it would fail if the same mistake were made
on Unix.

> Consistency in errors should focus on external errors.  Developer errors
> which don't fail for two, maybe three calls down the line require a little
> extra effort at debugging, but they usually pop right out when the developer
> steps the offending code.

This is very simple in my mind.  APR's goal is to get code that compiles
on all platforms and works the same on all platforms.  To that goal, the
behavior of the APIs must be as close to identical as possible.  The tests
that you are changing help to define what we expect to receive from our
functions when they fail for some reason.  You are removing those checks,
thus making APR behave differently on different platforms.  That is bad.

Now, having said that, there are places where APR does behave differently
on different platforms.  We fix that by providing _feature_ checks for
those behaviors that we can't emulate on all platforms.  This is
emulatable, and thus should be in the test suite.

BTW, we aren't talking about a developer error that won't fail for two or
three calls down the road.  We are talking about a porting error.  If you
call apr_file_open on Windows without APR_READ or APR_WRITE, the call will
work.  When you then take that exact same code to Unix, the apr_file_open
call will _fail_.  The goal of the test suite is to make sure that APR
behaves the same on all platforms.  After this change, it no longer does.

> >The problem with POSIX, is that there are hundreds of different versions
> >of POSIX, because no two POSIX implementations are 100% identical.  If
> >APR's functions and error conditions aren't identical, then you are just
> >asking the programmer to learn one more library that has it's own porting
> >secrets.  The whole point of APR is that programmers don't need to worry
> >about porting, APR did the work for them.
> If a platform truly allows read or write access without the APR_READ or
> APR_WRITE flag, that could be a bug.  But that wasn't the test I removed.
> If you care to introduce a test that succeeds regardless of the apr_file_open()
> results, and fails if apr_file_open() and apr_file_read() succeed without the
> APR_READ bit, that test I could support.  *That* would be a platform
> inconsistency we need to avoid.

I have no clue where that idea came from.  Is it a good test?  Of course,
please add it.  But that isn't what we are talking about at all.

> >-1 (none veto) to this change.
> That's fine; feel free to back up your vote with the necessary refactoring to
> support it.  Don't forget this change may be necessary to some unices
> when we start dealing with ACLs and other inode-metadata operations
> internally, which aren't based on reading or writing the data stream (fork)
> of the file.

I have no problem fixing the bug on Windows.  I have done so already in my
environment, but I haven't had time to test it thoroughly enough to
believe that it is the correct fix.


View raw message