httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r599385 - in /httpd/httpd/trunk: ./ modules/ssl/
Date Thu, 29 Nov 2007 22:06:42 GMT


On 11/29/2007 10:03 PM, Joe Orton wrote:
> On Thu, Nov 29, 2007 at 09:02:43PM +0100, Ruediger Pluem wrote:
>>> ==============================================================================
>>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>>> +++ httpd/httpd/trunk/CHANGES [utf-8] Thu Nov 29 03:18:40 2007
>>> @@ -2,6 +2,9 @@
>>>  Changes with Apache 2.3.0
>>>  [ When backported to 2.2.x, remove entry from this file ]
>>>  
>>> +  *) mod_ssl: Add support for OCSP validation of client certificates.
>>> +     PR 41123.  [Marc Stern <marc.stern approach.be>, Joe Orton]
>>> +
>> Shouldn't we add Steve to this? As far as I followed the discussion in 
>> Bugzilla he contributed very valuable points and we have credited 
>> people for less in the past :-).
> 
> Discussing this stuff makes me slightly uncomfortable, but I try to 
> follow a strict rule: credit in CHANGES exactly the set of people who 
> contributed actual lines of code.  Steve was credited in the commit 
> messages as a reviewer.  Such review is invaluable, no argument at all 
> :)

This rule is fine with me. I guess everybody has its own ruleset here which
might be slightly different.
I guess Steve is famous enough such that he can live without a CHANGES entry
and "only" a commit message :).


> 
> ...
>>> +/* Send the OCSP request serialized into BIO 'request' to the
>>> + * responder at given server given by URI.  Returns socket object or
>>> + * NULL on error. */
>>> +static apr_socket_t *send_request(BIO *request, const apr_uri_t *uri, 
>>> +                                  conn_rec *c, apr_pool_t *p)
>>> +{
>>> +    apr_status_t rv;
>>> +    apr_sockaddr_t *sa;
>>> +    apr_socket_t *sd;
>>> +    char buf[HUGE_STRING_LEN];
>> Is it really a good idea to store this on the stack? Shouldn't we allocate
>> this from the pool?
> 
> Hmmm, "shrug" - stack is cheaper than heap.

Just thought of platforms with small default stack sizes.
So lets stick to it until someone reports problems.

> 
> ...
>>> +    while ((len = BIO_read(request, buf, sizeof buf)) > 0) {
>>> +        char *wbuf = buf;
>>> +        apr_size_t remain = len;
>>> +        
>>> +        do {
>>> +            apr_size_t wlen = remain;
>>> +
>>> +            rv = apr_socket_send(sd, wbuf, &wlen);
>>> +            wbuf += remain;
>>> +            remain -= wlen;
>>> +        } while (rv == APR_SUCCESS && remain > 0);
>> Why do we need remain here and do not use len directly?
> 
> Really only to make the types match; len is an int but wlen must be an 
> apr_size_t.

Thanks for explaining.


> ...
>>> +    apr_brigade_destroy(bb);
>>> +    apr_brigade_destroy(tmpbb);
>> We miss to destroy the brigades in the cases above where we return NULL.
> 
> They are allocated out of the pool, so it Shouldn't Matter (TM).

Ok. They are taken from the newly created vpool. So it shouldn't really matter.

Regards

RĂ¼diger


Mime
View raw message