Re: svn commit: r951896 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_io.c ssl_engine_kernel.c
On Wednesday 09 June 2010, Ruediger Pluem wrote:
> On 06/09/2010 09:55 PM, Stefan Fritsch wrote:
> > On Wed, 9 Jun 2010, Ruediger Pluem wrote:
> >> On 06/06/2010 07:01 PM, s...@apache.org wrote:
> >>> Author: sf
> >>> Date: Sun Jun 6 17:01:29 2010
> >>> New Revision: 951896
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=951896&view=rev
> >>> Log:
> >>> Use new loglevel accessor macros to simplify code
> >>>
> >>> Modified:
> >>> httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> >>> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
> >>>
> >>> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> >>> URL:
> >>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_
> >>> engine_io.c?rev=951896&r1=951895&r2=951896&view=diff
> >>>
> >>> ===
> >>> ===
> >>>
> >>> --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
> >>> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Sun Jun 6
> >>> 17:01:29 2010
> >>> @@ -1015,7 +1015,7 @@ static void ssl_filter_io_shutdown(ssl_f
> >>>
> >>> SSL_smart_shutdown(ssl);
> >>>
> >>> /* and finally log the fact that we've closed the
> >>> connection */
> >>>
> >>> -if (mySrvFromConn(c)->loglevel >= APLOG_INFO) {
> >>> +if (APLOGcinfo(c)) {
> >>
> >> IMHO you need to use APLOGinfo(mySrvFromConn(c)) as otherwise
> >> you might choose the base_server
> >> which might not be what mySrvFromConn(c) returns.
> >
> > I think it is even more complicated than that and it affects many
> > places in mod_ssl. If c has a loglevel configuration, we should
> > probably use that. If not, we should use mySrvFromConn(c). But
> > in both cases, the log message should be tied to the connection,
> > to ensure that things like the client IP are logged. Maybe we
> > need a ap_log_scerror that accepts both a server_rec and
> > conn_rec.
>
> Makes sense.
Done in r954611.
>
> > AIUI, c->base_server is the default virtual host for the relevant
> > IP/port. Without SNI, mySrvFromConn(c) is the same but with SNI,
> > mySrvFromConn(c) may be a different name-based virtual host with
> > the same IP/port. So, mySrvFromConn(c) is basically r->server?
> > Is that correct?
>
> Yes, but a this point of time we may not have a request (yet), so
> mySrvFromConn(c) is the same as what r->server will be later
> during request processing.
>
> Regards
>
> Rüdiger
Re: svn commit: r951896 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_io.c ssl_engine_kernel.c
On 06/09/2010 09:55 PM, Stefan Fritsch wrote:
> On Wed, 9 Jun 2010, Ruediger Pluem wrote:
>> On 06/06/2010 07:01 PM, s...@apache.org wrote:
>>> Author: sf
>>> Date: Sun Jun 6 17:01:29 2010
>>> New Revision: 951896
>>>
>>> URL: http://svn.apache.org/viewvc?rev=951896&view=rev
>>> Log:
>>> Use new loglevel accessor macros to simplify code
>>>
>>> Modified:
>>> httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
>>> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
>>>
>>> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
>>> URL:
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c?rev=951896&r1=951895&r2=951896&view=diff
>>>
>>> ==
>>>
>>> --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
>>> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Sun Jun 6 17:01:29
>>> 2010
>>> @@ -1015,7 +1015,7 @@ static void ssl_filter_io_shutdown(ssl_f
>>> SSL_smart_shutdown(ssl);
>>>
>>> /* and finally log the fact that we've closed the connection */
>>> -if (mySrvFromConn(c)->loglevel >= APLOG_INFO) {
>>> +if (APLOGcinfo(c)) {
>>
>> IMHO you need to use APLOGinfo(mySrvFromConn(c)) as otherwise you
>> might choose the base_server
>> which might not be what mySrvFromConn(c) returns.
>
> I think it is even more complicated than that and it affects many places
> in mod_ssl. If c has a loglevel configuration, we should probably use
> that. If not, we should use mySrvFromConn(c). But in both cases, the log
> message should be tied to the connection, to ensure that things like the
> client IP are logged. Maybe we need a ap_log_scerror that accepts both a
> server_rec and conn_rec.
Makes sense.
>
> AIUI, c->base_server is the default virtual host for the relevant
> IP/port. Without SNI, mySrvFromConn(c) is the same but with SNI,
> mySrvFromConn(c) may be a different name-based virtual host with the
> same IP/port. So, mySrvFromConn(c) is basically r->server? Is that correct?
Yes, but a this point of time we may not have a request (yet), so
mySrvFromConn(c)
is the same as what r->server will be later during request processing.
Regards
Rüdiger
Re: svn commit: r951896 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_io.c ssl_engine_kernel.c
On Wed, 9 Jun 2010, Ruediger Pluem wrote:
On 06/06/2010 07:01 PM, s...@apache.org wrote:
Author: sf
Date: Sun Jun 6 17:01:29 2010
New Revision: 951896
URL: http://svn.apache.org/viewvc?rev=951896&view=rev
Log:
Use new loglevel accessor macros to simplify code
Modified:
httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c?rev=951896&r1=951895&r2=951896&view=diff
==
--- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Sun Jun 6 17:01:29 2010
@@ -1015,7 +1015,7 @@ static void ssl_filter_io_shutdown(ssl_f
SSL_smart_shutdown(ssl);
/* and finally log the fact that we've closed the connection */
-if (mySrvFromConn(c)->loglevel >= APLOG_INFO) {
+if (APLOGcinfo(c)) {
IMHO you need to use APLOGinfo(mySrvFromConn(c)) as otherwise you might choose
the base_server
which might not be what mySrvFromConn(c) returns.
I think it is even more complicated than that and it affects many places
in mod_ssl. If c has a loglevel configuration, we should probably use
that. If not, we should use mySrvFromConn(c). But in both cases, the log
message should be tied to the connection, to ensure that things like the
client IP are logged. Maybe we need a ap_log_scerror that accepts both a
server_rec and conn_rec.
AIUI, c->base_server is the default virtual host for the relevant IP/port.
Without SNI, mySrvFromConn(c) is the same but with SNI, mySrvFromConn(c)
may be a different name-based virtual host with the same IP/port. So,
mySrvFromConn(c) is basically r->server? Is that correct?
ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c,
"Connection closed to child %ld with %s shutdown "
"(server %s)",
@@ -1740,8 +1740,7 @@ void ssl_io_filter_init(conn_rec *c, req
apr_pool_cleanup_register(c->pool, (void*)filter_ctx,
ssl_io_filter_cleanup, apr_pool_cleanup_null);
-if ((s->loglevel >= APLOG_DEBUG)
- && (sc->ssl_log_level >= SSL_LOG_IO)) {
+if (APLOGcdebug(c) && (sc->ssl_log_level >= SSL_LOG_IO)) {
Same as above.
BIO_set_callback(SSL_get_rbio(ssl), ssl_io_data_cb);
BIO_set_callback_arg(SSL_get_rbio(ssl), (void *)ssl);
}
Regards
Rüdiger
Re: svn commit: r951896 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_io.c ssl_engine_kernel.c
On 06/06/2010 07:01 PM, s...@apache.org wrote:
> Author: sf
> Date: Sun Jun 6 17:01:29 2010
> New Revision: 951896
>
> URL: http://svn.apache.org/viewvc?rev=951896&view=rev
> Log:
> Use new loglevel accessor macros to simplify code
>
> Modified:
> httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
>
> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c?rev=951896&r1=951895&r2=951896&view=diff
> ==
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Sun Jun 6 17:01:29 2010
> @@ -1015,7 +1015,7 @@ static void ssl_filter_io_shutdown(ssl_f
> SSL_smart_shutdown(ssl);
>
> /* and finally log the fact that we've closed the connection */
> -if (mySrvFromConn(c)->loglevel >= APLOG_INFO) {
> +if (APLOGcinfo(c)) {
IMHO you need to use APLOGinfo(mySrvFromConn(c)) as otherwise you might choose
the base_server
which might not be what mySrvFromConn(c) returns.
> ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, c,
>"Connection closed to child %ld with %s shutdown "
>"(server %s)",
> @@ -1740,8 +1740,7 @@ void ssl_io_filter_init(conn_rec *c, req
> apr_pool_cleanup_register(c->pool, (void*)filter_ctx,
>ssl_io_filter_cleanup, apr_pool_cleanup_null);
>
> -if ((s->loglevel >= APLOG_DEBUG)
> - && (sc->ssl_log_level >= SSL_LOG_IO)) {
> +if (APLOGcdebug(c) && (sc->ssl_log_level >= SSL_LOG_IO)) {
Same as above.
> BIO_set_callback(SSL_get_rbio(ssl), ssl_io_data_cb);
> BIO_set_callback_arg(SSL_get_rbio(ssl), (void *)ssl);
> }
>
Regards
Rüdiger
