You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@attglobal.net> on 2002/05/14 16:45:34 UTC
how to log apr_status_t diagnostics in mod_ssl...
mod_ssl uses ssl_log(), which doesn't allow the caller to pass in
apr_status_t. Should we add a new parameter for that, or should we
add a separate function like ssl_log_error() with such a parameter?
I'd vote for adding a new parm to ssl_log() right after the 2nd
parameter (and yes, I'll make the changes to the many callers :) ).
Also, the existing accesses to errno in ssl_log() should be yanked, or
perhaps replaced with appropriate logic to handle whether or not an
apr_status_t was passed in.
Comments?
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
Re: how to log apr_status_t diagnostics in mod_ssl...
Posted by Greg Stein <gs...@lyra.org>.
On Tue, May 14, 2002 at 09:29:47AM -0700, Justin Erenkrantz wrote:
> On Tue, May 14, 2002 at 08:13:00AM -0700, Ryan Bloom wrote:
> > module's logic tends to lag behind. If you wanted to change all ssl_log
> > calls to ap_log_[r]error, I would be +1 for that too.
>
> +1 to that. I'd rather see us dump ssl_log() in favor of using our
> standard logging code - ap_log_[r]error. I think it made sense to
> have it log to a separate file when it wasn't part of the core, but
> now that we distribute it, I believe it should follow our standard
> logging conventions.
>
> My $.02. -- justin
I'll call your $.02 and raise you another $.02
+1, ditto on all the above.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: how to log apr_status_t diagnostics in mod_ssl...
Posted by Justin Erenkrantz <je...@apache.org>.
On Tue, May 14, 2002 at 08:13:00AM -0700, Ryan Bloom wrote:
> module's logic tends to lag behind. If you wanted to change all ssl_log
> calls to ap_log_[r]error, I would be +1 for that too.
+1 to that. I'd rather see us dump ssl_log() in favor of using our
standard logging code - ap_log_[r]error. I think it made sense to
have it log to a separate file when it wasn't part of the core, but
now that we distribute it, I believe it should follow our standard
logging conventions.
My $.02. -- justin
Re: how to log apr_status_t diagnostics in mod_ssl...
Posted by Jeff Trawick <tr...@attglobal.net>.
Justin Erenkrantz <je...@apache.org> writes:
> On Wed, May 15, 2002 at 08:11:07AM -0400, Jeff Trawick wrote:
> > If you look at ssl_log(), you'll find some pretty extensive support
> > for special features that ap_log_[r]error() and apr_strerror()
> > probably shouldn't ever have to know about.
> >
> > Consider the SSL_ADD_SSLERR flag... It leads to a lookup and log
>
> True. I was looking at it last night and the SSL_ADD_SSLERR flag
> is the one that worries me the most. Perhaps we should add a
> helper function that just logs the SSL error and annotation via
> ap_log_error()? So, the module could do:
>
> ap_log_error(APLOG_MARK, APLOG_ERR, rv, c->base_server,
> "Something bad happened. Details follow.");
> ssl_log_sslerr(c->base_server);
I can deal with that...
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
Re: how to log apr_status_t diagnostics in mod_ssl...
Posted by Justin Erenkrantz <je...@apache.org>.
On Wed, May 15, 2002 at 08:11:07AM -0400, Jeff Trawick wrote:
> If you look at ssl_log(), you'll find some pretty extensive support
> for special features that ap_log_[r]error() and apr_strerror()
> probably shouldn't ever have to know about.
>
> Consider the SSL_ADD_SSLERR flag... It leads to a lookup and log
True. I was looking at it last night and the SSL_ADD_SSLERR flag
is the one that worries me the most. Perhaps we should add a
helper function that just logs the SSL error and annotation via
ap_log_error()? So, the module could do:
ap_log_error(APLOG_MARK, APLOG_ERR, rv, c->base_server,
"Something bad happened. Details follow.");
ssl_log_sslerr(c->base_server);
where ssl_log_ssl_err() does something like:
{
unsigned long e;
char *err, *annotation;
e = ERR_get_error();
err = ERR_error_string(e, NULL);
annotation = ssl_log_annotation(e);
ap_log_error(APLOG_MARK, APLOG_ERR|APLOG_NOERRNO, 0, s,
"SSL Library Error: %ld %s %s", e, err, annotation);
}
(BTW, ssl_log_annotation should be modified to return the static
string "" if it can't find an annotation.)
I'd much prefer splitting it out in some manner like this rather
than keeping the ssl_log function. The ssl_log function is horrific.
What do you think? -- justin
Re: how to log apr_status_t diagnostics in mod_ssl...
Posted by Jeff Trawick <tr...@attglobal.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> > From: trawick@rdu88-251-253.nc.rr.com [mailto:trawick@rdu88-251-
> > 253.nc.rr.com] On Behalf Of Jeff Trawick
> >
> > mod_ssl uses ssl_log(), which doesn't allow the caller to pass in
> > apr_status_t. Should we add a new parameter for that, or should we
> > add a separate function like ssl_log_error() with such a parameter?
> >
> > I'd vote for adding a new parm to ssl_log() right after the 2nd
> > parameter (and yes, I'll make the changes to the many callers :) ).
> >
> > Also, the existing accesses to errno in ssl_log() should be yanked, or
> > perhaps replaced with appropriate logic to handle whether or not an
> > apr_status_t was passed in.
> >
> > Comments?
>
> If you wanted to change all ssl_log
> calls to ap_log_[r]error, I would be +1 for that too.
If you look at ssl_log(), you'll find some pretty extensive support
for special features that ap_log_[r]error() and apr_strerror()
probably shouldn't ever have to know about.
Consider the SSL_ADD_SSLERR flag... It leads to a lookup and log
of one or more SSL errors. This definitely belongs in a utility
routine like ssl_log(). SSL_LOG_INIT is a less interesting feature
which saves some callers a bit of work (but not a critical feature).
This isn't a piece of code I'm very familiar with, and I would not be
at all shocked if there is complexity there that is not necessary in
real life, but for now it looks like the following is appropriate:
if special features are used like SSL_ADD_SSLERR:
keep calling ssl_log()
(it remains to be seen whether or not ssl_log() needs an
apr_status_t parameter; I doubt it since the SSL libraries don't
use APR and APR doesn't make room for an SSL library error code in
apr_status_t)
if ssl_log(...SSL_LOG_TRACE...)
-> ap_log_error(...APLOG_DEBUG...)
if ssl_log(...SSL_LOG_ERR...)
-> ap_log_error(...APLOG_ERR...)
Meanwhile rip out the support for tracing to a separate file since
that is effectively broken with the translation above.
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
RE: how to log apr_status_t diagnostics in mod_ssl...
Posted by Ryan Bloom <rb...@covalent.net>.
> From: trawick@rdu88-251-253.nc.rr.com [mailto:trawick@rdu88-251-
> 253.nc.rr.com] On Behalf Of Jeff Trawick
>
> mod_ssl uses ssl_log(), which doesn't allow the caller to pass in
> apr_status_t. Should we add a new parameter for that, or should we
> add a separate function like ssl_log_error() with such a parameter?
>
> I'd vote for adding a new parm to ssl_log() right after the 2nd
> parameter (and yes, I'll make the changes to the many callers :) ).
>
> Also, the existing accesses to errno in ssl_log() should be yanked, or
> perhaps replaced with appropriate logic to handle whether or not an
> apr_status_t was passed in.
>
> Comments?
Fix the ssl_log API so that it accepts the apr_status_t. This is one of
the problems with having modules that use their own logs to store error
information. When the core API changes to log more information, the
module's logic tends to lag behind. If you wanted to change all ssl_log
calls to ap_log_[r]error, I would be +1 for that too.
Ryan