Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: domain of jorton@redhat.com designates
 66.187.233.31 as permitted sender)
Date: Thu, 29 Nov 2007 21:03:29 +0000
From: Joe Orton <jorton@redhat.com>
To: dev@httpd.apache.org
Subject: Re: svn commit: r599385 - in /httpd/httpd/trunk: ./ modules/ssl/
Message-ID: <20071129210329.GA24695@redhat.com>
Mail-Followup-To: dev@httpd.apache.org
References: <20071129111843.69CBA1A9832@eris.apache.org>
 <474F1AE3.5050902@apache.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <474F1AE3.5050902@apache.org>
User-Agent: Mutt/1.5.17 (2007-11-01)
Organization: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor,
 Berkshire, SL4 1TE, United Kingdom.
  Registered in UK and Wales under Company Registration No. 03798903
  Directors: Michael Cunningham (USA), Brendan Lane (Ireland),
 Matt Parson (USA), Charlie Peters (USA)

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 
:)

...
> > +/* 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.

...
> > +    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.

...
> > +    count = 0;
> > +    while ((line = get_line(tmpbb, bb, c, p)) != NULL && line[0]
> > +           && ++count < MAX_HEADERS) {
> > +        ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
> > +                      "OCSP response header: %s", line);
> > +    }
> 
> Why don't we exit with an error message if count > MAX_HEADERS?

Good catch, will fix.

...
> > +    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).

Thanks for the review!

joe