Mailing-List: contact cvs-help@apr.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@apr.apache.org
Date: Sun, 29 Dec 2002 10:48:24 -0800 (PST)
From: <rbb@apache.org>
To: "William A. Rowe, Jr." <wrowe@apache.org>
cc: <rbb@apache.org>,  <dev@apr.apache.org>,  <apr-cvs@apache.org>
Subject: Re: cvs commit: apr/test testdso.c
In-Reply-To: <5.1.0.14.2.20021229110740.03744d58@pop3.rowe-clan.net>
Message-ID: <Pine.LNX.4.33L2.0212291006010.22992-100000@vangogh.rkbloom.net>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII


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

> At 12:21 AM 12/29/2002, rbb@apache.org wrote:
>
> >On 29 Dec 2002 wrowe@apache.org wrote:
> >
> >> wrowe       2002/12/28 21:44:02
> >>
> >>     First; once any apr object is closed, the results are undefined.
> >...
> >>   Index: testdso.c
> >>   --- testdso.c       19 Dec 2002 16:15:29 -0000      1.30
> >>   +++ testdso.c       29 Dec 2002 05:44:01 -0000      1.31
> >>   ...
> >>   @@ -155,17 +155,11 @@
> >>
> >>        status = apr_dso_unload(h);
> >>        CuAssert(tc, apr_dso_error(h, errstr, 256), APR_SUCCESS == status);
> >>   -
> >>   -    status = apr_dso_sym(&func1, h, "print_hello");
> >>   -    CuAssertIntEquals(tc, APR_EINIT, status);
> >>    }
> >
> >I seriously disagree with this change.
>
>
> >If the dso is unloaded, you
> >shouldn't be able to find a symbol in it anymore.
>
> I agree with you.  However, h is now undefined.  This means that we
> will generally react by segfaulting or throwing any indeterminate error.
> EINIT is a Unixism.

There is no way at all that this code will segfault.  Not on any platform,
because the unload only unloaded the native instance of the library, it
had no effect on the APR wrapper.

> >One of the points of
> >APR is that it removes this kind of platform difference.  Right now,
> >somebody can program on Windows, and use the wrong pool to load a library.
> >Then, after the pool is cleared still be able to dso_sym that library.
>
> Why do you suppose they can do that?  Win32 returns an error, just
> not that specific error.  The test failed because the result was not
> EINIT.  Once a file/dso/shm/pipe/socket etc is closed/unloaded, that's
> it, game over.  How a platform expresses the error is undefined.

If the error was the wrong one to check for, then change it to look for
APR_STATUS_IS_foo, and make foo work.  The error can't be undefined,
because if it is then APR is useless.   Throughout the code, we have
different error codes from different platforms, and we handle it
gracefully, why aren't we handling it gracefully in this case.

As for the game being over once a file/dso/shm/pipe/socket is
closed/unloaded, that is hogwash.  We test for this sort of thing
throughout the test suite.

> >I am currently -0.9 for this change.  Is there a reason we can't solve
> >this problem so that incorrect pool scope can be easily found on all
> >platforms?
>
> That's a bogus arguement.  If h is within a more-limited pool scope
> than the programmer intended, then the next apr_dso_sym() *will*
> segfault, and not return some gracious EINIT.

It isn't seg faulting.  If it does seg fault, then the platform has broken
code.  As it happens, Windows is returning an error.  The test was wrong,
but the spirit of the test was correct.  Fix the code.  Don't break the
test.

I am now -1 for this change.  It is bogus.  There is a status code
returned for all platforms in this case, to not test for it is doing a
huge disservice to our users.

> If you want to define the behavior of all functions following closure or
> unloading, then you must go through all platforms and introduce tests
> for a valid handle on all operations.  I really think that is overkill and
> undermines the original design constraints.

What are you talking about?  This statement says "We have no idea what
happens if you try to operate on an APR type that you have already closed.
So, rather than fix that problem, let's just call it 'undefined'."  That
is completely unfair to users.  Especially since we do know what will
happen.  The internal type will be closed, but the APR wrapper will
survive just fine.  All we need to do is cleanly define APR_STATUS_IS_foo
macros to handle this case.  Doing any less is just being lazy.

> However, as I suggested when we first argued the issue of argument
> checking, it isn't unreasonable to have small sections such as...
>
> APR_DECLARE(apr_status_t) apr_foo_fn(apr_foo_t *t)
> {
> #ifdef DEBUG_ARGS
>     if (!t)
>         return APR_EINVAL;
>     if (!t->ptrmbr)
>         return APR_EINIT;
> #endif

I am thoroughly against code like this.  If the args are worth checking in
debug mode, then they are worth checking in production code.  To say that
we don't check args is false BTW.  We don't check all args.  In general,
we don't check arguments that may be invalid due to programmer
incompetance.  We do check args where not checking them would cause
horrible effects and the programmer couldn't have predicted the results.
The basic rule (from way back in the httpd days), is that argument
checking is a waste of time because the programmer should find the mistake
in their testing, and it is better to seg fault for the programmer than to
return an error.  A seg fault is hard to get around.  However, what we are
talking about here isn't a seg fault.  It is different responses on
different systems, which is what APR is trying to solve.  Your change to
the test suite makes APR _less_ portable, which is why it is wrong.

> It's all in the distinction between *defined* behavior, which can be tested,
> and *undefined* behavior which is untestable.  {Unless we can test sevg
> as a success case, perhaps using STL exceptions.}

Show me one place that we are currently testing a segv, and this statement
applies.  Otherwise, it is just FUD.

Ryan