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
