Jim,
I'm thinking along the same lines. The while {1} thing is similar to a
try block, but it handles the special case where there is really only on
exception, on error or a successful return, fast-forward to
traces...actually check for an error then maybe do traces.
What I'm not sure about is some of the looping which happens with
redirects. Specifically redirecting to non-200 codes. I'm not sure why
there is a need to reauthorize before running client or server error
handlers. In essence there is a chance that the original failure will be
lost in this process and a new failure will replace it.
***Digression***
I seem to remember that you once said it would be nice if there were
more filter points available.
What would be interesting is if you could fast-forward, or rewind to
more points. Right now we have NS_FILTER_BREAK, NS_FILTER_RETURN, NS_OK
and NS_ERROR, which all go to pre-determined points. These really are
not states, just a rough indication of something.
Right now we can rewind to authorization, but then we skip post-auth
filters and run the new request.
Anyway, it seems like the while loop could change to some kind of
generic state machine.
Instead of a fixed set of steps, each step could potentially end with a
restart with a new set of steps, or just a restart of the same set. You
could use this to avoid running through the list of filters for urls
which will never match the current url/method.
Maybe I can do a mock-up in Tcl.
***End Digression***
Anyway, here is where I'm at with the error loop:
After a few additional changes I have got one thing working: all errors
are handled after the while {1} loop. I have one remaining problem with
Ns_ConnReturnInternalError. It looks like this:
Ns_ConnReturnInternalError(Ns_Conn *conn)
{
int result;
Ns_SetTrunc(conn->outputheaders, 0);
if (ReturnRedirect(conn, 500, &result)) {
return result;
}
return Ns_ConnReturnNotice(conn, 500, "Server Error",
"The requested URL cannot be accessed "
"due to a system error on this server.");
}
The problem is that the "result" is returned base upon the existence of
a redirect mapping. This works for everything except errors which take
place when trying to run the configured error handler.
This case highlights exactly what you are talking about. The result is
an error, but ReturnRedirect found a handler. Why did the handler fail?
In the bug I'm working on, the request line is "PROPFIND /abc HTTP/1.1"
It seems like the correct error is a 501 "Not Implemented", but that
error should have shown up before the 404 redirect.
I'm thinking the problem is in ReturnRedirect and how redirect are
mapped (they leave out the method). Adding a method in the redirect
mapping would be interesting, allowing different handlers for different
methods.
The other thing I'm looking at is trying to short circuit the redirect
code so that it doesn't redirect to the same url.
tom jackson
On Wed, 2009-04-15 at 13:55 -0400, Jim Davidson wrote:
> I'm wondering if there needs to be some more specific exception flags
> stored in the connection structure to handle all these cases and some
> C and Tcl api's to access/modify the same. It seems you're doing the
> good work of rationalizing all the error conditions that had been
> pretty confused together and shuffling / updating the code to handle
> the various cases but more explicit tracking of the error conditions
> (execution, pre/post filter, connection, auth, etc.) could make things
> easier. This may lead the code to look like more try/catch type
> exception handling instead of a non-obvious code path understandable
> only with the surrounding comments.
>
> -Jim
>
>
>
> On Apr 15, 2009, at 11:12 AM, Tom Jackson wrote:
>
> > Gustaf,
> > Thanks for testing on a production server. I haven't tested this with
> > the authorization module, but it seems like it should work okay.
> >
> > The internal server loop bug that I tracked down yesterday led me to
> > another function which contains code very similar to the new
> > ConnAuthorize function, so I'm wondering if I should see if I should
> > maybe export ConnAuthorize to NsConnAuthorize. The overall idea is to
> > only run Ns_ConnReturnInternalError at the end of the request and not
> > from inside the authorization or redirection functions (because the
> > returns Ns_ConnReturnForbidden and Ns_ConnReturnUnauthorized could
> > also
> > return NS_ERROR.
> >
> > Any ideas are welcome. (I think just removing
> > Ns_ConnReturnInternalError
> > from Ns_ConnRedirect will break the loop.)
> >
> > tom jackson
> >
> > Here's the two chunks of code:
> >
> > static int
> > ConnAuthorize(Conn *connPtr)
> > {
> > Ns_Conn *conn = (Ns_Conn *) connPtr;
> > NsServer *servPtr = connPtr->servPtr;
> > int status;
> >
> > status = Ns_AuthorizeRequest(servPtr->server,
> > connPtr->request->method, connPtr->request->url,
> > connPtr->authUser, connPtr->authPasswd, connPtr->peer);
> >
> > switch (status) {
> > case NS_OK:
> > break;
> > case NS_FORBIDDEN:
> > if ((status = Ns_ConnReturnForbidden(conn)) == NS_OK) {
> > status = NS_FILTER_RETURN;
> > }
> > break;
> > case NS_UNAUTHORIZED:
> > if ((status = Ns_ConnReturnUnauthorized(conn)) == NS_OK) {
> > status = NS_FILTER_RETURN;
> > }
> > break;
> > case NS_ERROR:
> > default:
> > status = NS_ERROR;
> > break;
> > }
> >
> > return status;
> > }
> >
> > and from op.c:
> >
> > int
> > Ns_ConnRedirect(Ns_Conn *conn, char *url)
> > {
> > Conn *connPtr = (Conn *) conn;
> > int status;
> >
> > ++connPtr->recursionCount;
> >
> > /*
> > * Update the request URL.
> > */
> >
> > Ns_SetRequestUrl(conn->request, url);
> >
> > /*
> > * Re-authorize and run the request.
> > */
> >
> > status = Ns_AuthorizeRequest(Ns_ConnServer(conn), conn->request-
> > >method,
> > conn->request->url, conn->authUser,
> > conn->authPasswd, Ns_ConnPeer(conn));
> > switch (status) {
> > case NS_OK:
> > status = Ns_ConnRunRequest(conn);
> > break;
> > case NS_FORBIDDEN:
> > status = Ns_ConnReturnForbidden(conn);
> > break;
> > case NS_UNAUTHORIZED:
> > status = Ns_ConnReturnUnauthorized(conn);
> > break;
> > case NS_ERROR:
> > default:
> > status = Ns_ConnReturnInternalError(conn);
> > break;
> > }
> >
> > return status;
> > }
> >
> >
> > On Wed, 2009-04-15 at 12:52 +0200, Gustaf Neumann wrote:
> > your rewrite of the two functions below look fine to me. The structure
> >> is much clearer now, results of the authorization handling are
> >> handled
> >> now consistently. I am useing these functions since a few days on one
> >> of my servers and found nothing unusual. Many thanks to Tom!
> >>
> >> Andrew, did you test this version as well for your test-cases?
> >> If thise works for you as well (i would assume so), i would think
> >> that version should go into CVS.
> >>
> >
> >
> > --
> > AOLserver - http://www.aolserver.com/
> >
> > To Remove yourself from this list, simply send an email to
> > <[email protected]
> > > with the
> > body of "SIGNOFF AOLSERVER" in the email message. You can leave the
> > Subject: field of your email blank.
>
>
> --
> AOLserver - http://www.aolserver.com/
>
> To Remove yourself from this list, simply send an email to
> <[email protected]> with the
> body of "SIGNOFF AOLSERVER" in the email message. You can leave the Subject:
> field of your email blank.
>
--
AOLserver - http://www.aolserver.com/
To Remove yourself from this list, simply send an email to
<[email protected]> with the
body of "SIGNOFF AOLSERVER" in the email message. You can leave the Subject:
field of your email blank.
