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
