You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2010/06/09 19:54:30 UTC
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, sf@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
Re: svn commit: r951896 - in /httpd/httpd/trunk/modules/ssl: ssl_engine_io.c ssl_engine_kernel.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
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, sf@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
Posted by Ruediger Pluem <rp...@apache.org>.
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, sf@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
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Wed, 9 Jun 2010, Ruediger Pluem wrote:
> On 06/06/2010 07:01 PM, sf@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
>