Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Content-Type: text/plain;
  charset="iso-8859-1"
From: Ryan Bloom <rbb@covalent.net>
Reply-To: rbb@covalent.net
Organization: Covalent Technologies
To: dev@httpd.apache.org, Justin Erenkrantz <jerenkrantz@ebuilt.com>
Subject: Re: [PATCH] Rework httpd buckets/filtering
Date: Sun, 23 Sep 2001 08:33:07 -0700
References: <20010923041203.H6756@ebuilt.com>
In-Reply-To: <20010923041203.H6756@ebuilt.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Message-Id: <20010923153307.AFA6346DFC@koj.rkbloom.net>


None of this can be committed until it has passed all of the tests in the perl test
framework. The last time somebody (me) mucked with these filters it broke all of the
CGI script handling. I would also like to hear from Jeff about this, because when we
wrote the filters originally, we had input filtering use this design, and Jeff and Greg
were adamant that filters needed to be able to return more than they were asked for.
I am uncomfortable committing this until they have a chance to look it over.

Other than that, I made some comments in-line that should be addressed. I will
try to run it through the test framework today or tomorrow, so that I can give it
a +1 or not.

Ryan


> Index: include/apr_buckets.h
> ===================================================================
> RCS file: /home/cvs/apr-util/include/apr_buckets.h,v
> retrieving revision 1.118
> diff -u -r1.118 apr_buckets.h
> --- include/apr_buckets.h	2001/09/22 22:36:07	1.118
> +++ include/apr_buckets.h	2001/09/23 10:36:12
> @@ -689,6 +689,17 @@
>                                               apr_off_t *length);
>
>  /**
> + * create a flat buffer of the elements in a bucket_brigade.
> + * @param b The bucket brigade to create the buffer from
> + * @param c The pointer to the returned character string
> + * @param len The length of the returned character string
> + * @param p The pool to allocate the character string from.
> + * Note: You *must* have enough space allocated to store all of the data.
> + * See apr_brigade_length().
> + */
> +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> char *c); +
> +/**

Please change the docs to match the API.

> Index: buckets/apr_brigade.c
> ===================================================================
> RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
> retrieving revision 1.25
> diff -u -r1.25 apr_brigade.c
> --- buckets/apr_brigade.c	2001/09/03 22:00:36	1.25
> +++ buckets/apr_brigade.c	2001/09/23 10:36:12
> @@ -221,6 +221,29 @@
>      return APR_SUCCESS;
>  }
>
> +APU_DECLARE(apr_status_t) apr_brigade_to_buffer(apr_bucket_brigade *b,
> char *c) +{
> +    apr_bucket *e;
> +    char *current;
> +    apr_size_t curlen;
> +    apr_status_t status;
> +
> +    current = c;
> +
> +    APR_BRIGADE_FOREACH(e, b) {
> +
> +        status = apr_bucket_read(e, (const char **)&current, &curlen,
> +                                 APR_BLOCK_READ);

This is most likely wrong. You probably want to do a non-blocking read, and
return immediately if you can't find any data. That way, the caller can keep working
with the data he has. You also probably want to split the brigade at that location,
so that the caller can keep track of where they are in the brigade.

> +        if (status != APR_SUCCESS)
> +            return status;
> +
> +        current += curlen;
> +    }
> +
> +    return APR_SUCCESS;
> +
> +}
> +
>  APU_DECLARE(apr_status_t) apr_brigade_to_iovec(apr_bucket_brigade *b,
>                                                 struct iovec *vec, int
> *nvec) {
> Index: buckets/apr_buckets_pipe.c
> ===================================================================
> RCS file: /home/cvs/apr-util/buckets/apr_buckets_pipe.c,v
> retrieving revision 1.41
> diff -u -r1.41 apr_buckets_pipe.c
> --- buckets/apr_buckets_pipe.c	2001/09/22 22:36:07	1.41
> +++ buckets/apr_buckets_pipe.c	2001/09/23 10:36:13
> @@ -106,15 +106,8 @@
>      }
>      else {
>          free(buf);
> -        a = apr_bucket_immortal_make(a, "", 0);
> -        *str = a->data;
> -        if (rv == APR_EOF) {
> +        if (rv == APR_EOF)
>              apr_file_close(p);
> -            if (block == APR_NONBLOCK_READ) {
> -                /* XXX: this is bogus, should return APR_SUCCESS */
> -                return APR_EOF;
> -            }
> -        }

You can't do this.  The reason for the immortal bucket, was that we are closing 
the pipe or socket bucket.  What this change does, is it closes the pipe, but
leaves the bucket in the brigade.  This will cause problems later. You can't
just remove the bucket, because you only have access to the one bucket,
and if you remove this bucket, then you also need to update the pointer from
the previous bucket, which you can't do.

>      }
>      return APR_SUCCESS;
>  }
> Index: buckets/apr_buckets_socket.c
> ===================================================================
> RCS file: /home/cvs/apr-util/buckets/apr_buckets_socket.c,v
> retrieving revision 1.30
> diff -u -r1.30 apr_buckets_socket.c
> --- buckets/apr_buckets_socket.c	2001/09/22 22:36:07	1.30
> +++ buckets/apr_buckets_socket.c	2001/09/23 10:36:13
> @@ -79,7 +79,7 @@
>      }
>
>      if (rv != APR_SUCCESS && rv != APR_EOF) {
> -	free(buf);
> +        free(buf);
>          return rv;
>      }
>      /*
> @@ -109,12 +109,6 @@
>      }
>      else {
>          free(buf);
> -        a = apr_bucket_immortal_make(a, "", 0);
> -        *str = a->data;
> -        if (rv == APR_EOF && block == APR_NONBLOCK_READ) {
> -            /* XXX: this is bogus... should return APR_SUCCESS */
> -            return APR_EOF;
> -        }
>      }
>      return APR_SUCCESS;
>  }

Same as above.

> Index: modules/http/http_protocol.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
> retrieving revision 1.362
> diff -u -r1.362 http_protocol.c
> --- modules/http/http_protocol.c	2001/09/17 13:12:37	1.362
> +++ modules/http/http_protocol.c	2001/09/23 10:56:30
> @@ -487,6 +487,7 @@
>      enum {
>          WANT_HDR /* must have value zero */,
>          WANT_BODY,
> +        WANT_ENDCHUNK,
>          WANT_TRL
>      } state;
>  };
> @@ -498,9 +499,7 @@
>  {
>      apr_status_t rv;
>      struct dechunk_ctx *ctx = f->ctx;
> -    apr_bucket *b;
> -    const char *buf;
> -    apr_size_t len;
> +    apr_off_t len;
>
>      if (!ctx) {
>          f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(struct dechunk_ctx)); 
>  @@ -508,43 +507,38 @@
>
>      do {
>          if (ctx->chunk_size == ctx->bytes_delivered) {
> -            /* Time to read another chunk header or trailer...  ap_http_filter() is 
> -             * the next filter in line and it knows how to return a brigade with 
> -             * one line.
> -             */
> +            /* Time to read another chunk header or trailer...  We only want 
> +             * one line - by specifying 0 as the length to read.  */
>               char line[30];
>
> -            if ((rv = ap_getline(line, sizeof(line), f->r,
> -                                 0 /* readline */)) < 0) {
> +            if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
>                  return rv;
>              }
>              switch(ctx->state) {
>              case WANT_HDR:
>                  ctx->chunk_size = get_chunk_size(line);
>                  ctx->bytes_delivered = 0;
> -                if (ctx->chunk_size == 0) {
> +                if (ctx->chunk_size == 0)
>                      ctx->state = WANT_TRL;
> -                }
> -                else {
> +                else
>                      ctx->state = WANT_BODY;
> -                }

Don't do that.  The { and } aren't strictly required, but it is good practice
to have them. It makes people who are modifying the code later not have
to worry about them. It also shrinks the size of the patch, which is good for
people who have to review it. In this case, we would remove about 8 lines
from the size of the patch.  Not much, but every line that isn't in the patch
helps.

>                  break;
> +            case WANT_ENDCHUNK:
> +                /* We don't care.  Next state please. */
> +                ctx->state = WANT_TRL;
> +                break;
>              case WANT_TRL:
> -                /* XXX sanity check end chunk here */
> -                if (strlen(line)) {
> -                    /* bad trailer */
> -                }
> -                if (ctx->chunk_size == 0) { /* we just finished the last chunk? */
> -                    /* ### woah... ap_http_filter() is doing this, too */
> -                    /* append eos bucket and get out */
> -                    b = apr_bucket_eos_create();
> -                    APR_BRIGADE_INSERT_TAIL(bb, b);
> +                /* Keep reading until we get to a blank line. */
> +                /* Should add these headers to r->headers_in? */
> +                if (!strlen(line))
> +                {
> +                    ctx->state = WANT_HDR;
>                      return APR_SUCCESS;
>                  }
> -                ctx->state = WANT_HDR;
> -                break;
> +                break;
>              default:
> -                ap_assert(ctx->state == WANT_HDR || ctx->state == WANT_TRL); 
> +                ap_assert(ctx->state == WANT_HDR || ctx->state == WANT_TRL ||
> +                          ctx->state == WANT_ENDCHUNK);
>              }
>          }
>      } while (ctx->state != WANT_BODY);
> @@ -558,27 +552,11 @@
>              return rv;
>          }
>
> -        /* Walk through the body, accounting for bytes, and removing an eos
> -         * bucket if ap_http_filter() delivered the entire chunk. -   
>      *
> -         * ### this shouldn't be necessary. 1) ap_http_filter shouldn't be
> -         * ### adding EOS buckets. 2) it shouldn't return more bytes than
> -         * ### we requested, therefore the total len can be found with a
> -         * ### simple call to apr_brigade_length(). no further munging
> -         * ### would be needed.
> -         */
> -        b = APR_BRIGADE_FIRST(bb);
> -        while (b != APR_BRIGADE_SENTINEL(bb) && !APR_BUCKET_IS_EOS(b)) {
> -            apr_bucket_read(b, &buf, &len, mode);
> -            AP_DEBUG_ASSERT(len <= ctx->chunk_size -
> ctx->bytes_delivered); -            ctx->bytes_delivered += len;
> -            b = APR_BUCKET_NEXT(b);
> -        }
> -        if (ctx->bytes_delivered == ctx->chunk_size) {
> -            AP_DEBUG_ASSERT(APR_BUCKET_IS_EOS(b));
> -            apr_bucket_delete(b);
> -            ctx->state = WANT_TRL;
> -        }
> +        /* Have we read a complete chunk?  */
> +        apr_brigade_length(bb, 1, &len);
> +        ctx->bytes_delivered += len;
> +        if (ctx->bytes_delivered == ctx->chunk_size)
> +            ctx->state = WANT_ENDCHUNK;
>      }
>
>      return APR_SUCCESS;
> @@ -588,7 +566,8 @@
>      apr_bucket_brigade *b;
>  } http_ctx_t;
>
> -apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t mode, apr_off_t *readbytes) 
> +apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, 
> +                            ap_input_mode_t mode, apr_off_t *readbytes) {
>      apr_bucket *e;
>      char *buff;
> @@ -641,7 +620,8 @@
>      }
>
>      if (APR_BRIGADE_EMPTY(ctx->b)) {
> -        if ((rv = ap_get_brigade(f->next, ctx->b, mode, readbytes)) != APR_SUCCESS) {
> +        if ((rv = ap_get_brigade(f->next, ctx->b, mode,
> +                                 readbytes)) != APR_SUCCESS) {

Don't do this.  You have cluttered this patch with code re-formatting.  That is bad.
It makes people have to review more than is necessary.  Keep the patch as
small as possible by not re-formatting it as you go please.

>              return rv;
>          }
>      }
> @@ -655,12 +635,16 @@
>      if (*readbytes == -1) {
>          apr_bucket *e;
>          apr_off_t total;
> +        const char *str;
> +        apr_size_t len;
>          APR_BRIGADE_FOREACH(e, ctx->b) {
> -            const char *str;
> -            apr_size_t len;
> +            /* We don't care about these values.  We just want to force the
> +             * lower level to convert it.  This seems hackish. */
>              apr_bucket_read(e, &str, &len, APR_BLOCK_READ);

Why does this seem hack-ish?  We can't pass a socket up the filter stack
and we need to get all the data.  This is the defined way to do that.

>          }
>          APR_BRIGADE_CONCAT(b, ctx->b);
> +
> +        /* Force a recompute of the length */
>          apr_brigade_length(b, 1, &total);
>          *readbytes = total;
>          e = apr_bucket_eos_create();
> @@ -670,67 +654,40 @@
>      /* readbytes == 0 is "read a single line". otherwise, read a block. */
>      if (*readbytes) {
>          apr_off_t total;
> +        apr_bucket *e;
> +        apr_bucket_brigade *newbb;
>
> -        /* ### the code below, which moves bytes from one brigade to the
> -           ### other is probably bogus. presuming the next filter down was
> -           ### working properly, it should not have returned more than
> -           ### READBYTES bytes, and we wouldn't have to do any work.
> -        */
> -
> -        APR_BRIGADE_NORMALIZE(ctx->b);
> -        if (APR_BRIGADE_EMPTY(ctx->b)) {
> -            if ((rv = ap_get_brigade(f->next, ctx->b, mode, readbytes)) !=
> APR_SUCCESS) { -                return rv;
> -            }
> -        }
> -
> +        newbb = NULL;
> +
>          apr_brigade_partition(ctx->b, *readbytes, &e);
> -        APR_BRIGADE_CONCAT(b, ctx->b);
> +        /* Must do split before CONCAT */
>          if (e != APR_BRIGADE_SENTINEL(ctx->b)) {
> -            apr_bucket_brigade *temp;
> +            newbb = apr_brigade_split(ctx->b, e);
> +        }
> +        APR_BRIGADE_CONCAT(b, ctx->b);
>
> -            temp = apr_brigade_split(b, e);
> +        /* FIXME: Is this really needed?  Due to pointer use in sentinels,
> +         * I think so. */
> +        if (newbb)
> +            APR_BRIGADE_CONCAT(ctx->b, newbb);
>
> -            /* ### darn. gotta ensure the split brigade is in the proper pool.
> -               ### this is a band-aid solution; we shouldn't even be doing
> -               ### all of this brigade munging (per the comment above).
> -               ### until then, this will get the right lifetimes. */
> -            APR_BRIGADE_CONCAT(ctx->b, temp);
> -        }
> -        else {
> -            if (!APR_BRIGADE_EMPTY(ctx->b)) {
> -                ctx->b = NULL; /*XXX*/
> -            }
> -        }
> +        /* FIXME: Are we assuming that b is empty when we enter? */

You are in an input filter.  All brigades must be empty when passed to an
input filter.  Data is put into the filter on the way back up.

>          apr_brigade_length(b, 1, &total);
> -        *readbytes -= total;
> +        *readbytes = total;
>
> -        /* ### this is a hack. it is saying, "if we have read everything
> -           ### that was requested, then we are at the end of the request."
> -           ### it presumes that the next filter up will *only* call us
> -           ### with readbytes set to the Content-Length of the request.
> -           ### that may not always be true, and this code is *definitely*
> -           ### too presumptive of the caller's intent. the point is: this
> -           ### filter is not the guy that is parsing the headers or the
> -           ### chunks to determine where the end of the request is, so we
> -           ### shouldn't be monkeying with EOS buckets.
> -        */
> -        if (*readbytes == 0) {
> -            apr_bucket *eos = apr_bucket_eos_create();
> -
> -            APR_BRIGADE_INSERT_TAIL(b, eos);
> -        }
>          return APR_SUCCESS;
>      }
>
>      /* we are reading a single line, e.g. the HTTP headers */
>      while (!APR_BRIGADE_EMPTY(ctx->b)) {
>          e = APR_BRIGADE_FIRST(ctx->b);
> -        if ((rv = apr_bucket_read(e, (const char **)&buff, &len, mode)) != APR_SUCCESS) {
> +        if ((rv = apr_bucket_read(e, (const char **)&buff, &len,
> +                                  mode)) != APR_SUCCESS) {

Again, wasted reformatting.

>              return rv;
>          }
>
>          pos = memchr(buff, APR_ASCII_LF, len);
> +        /* We found a match. */
>          if (pos != NULL) {
>              apr_bucket_split(e, pos - buff + 1);
>              APR_BUCKET_REMOVE(e);
> @@ -740,6 +697,7 @@
>          APR_BUCKET_REMOVE(e);
>          APR_BRIGADE_INSERT_TAIL(b, e);
>      }
> +
>      return APR_SUCCESS;
>  }
>
> @@ -1517,14 +1475,12 @@
>   */
>  AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer,
> apr_size_t bufsiz) {
> -    apr_size_t len_read, total;
> -    apr_status_t rv;
> -    apr_bucket *b, *old;
> -    const char *tempbuf;
> +    apr_off_t len;
> +    apr_bucket *b;
>      core_request_config *req_cfg =
>  	(core_request_config *)ap_get_module_config(r->request_config,
>                                                      &core_module);
> -    apr_bucket_brigade *bb = req_cfg->bb;
> +    apr_bucket_brigade *bb = req_cfg->bb, *newbb;
>
>      do {
>          if (APR_BRIGADE_EMPTY(bb)) {
> @@ -1546,41 +1502,32 @@
>          return 0;
>      }
>
> -    /* ### it would be nice to replace the code below with "consume N bytes
> -       ### from this brigade, placing them into that buffer." there are
> -       ### other places where we do the same...
> -       ###
> -       ### alternatively, we could partition the brigade, then call a
> -       ### function which serializes a given brigade into a buffer. that
> -       ### semantic is used elsewhere, too...
> -    */
> -
> -    total = 0;
> -    while (total < bufsiz
> -           && b != APR_BRIGADE_SENTINEL(bb)
> -           && !APR_BUCKET_IS_EOS(b)) {
> -
> -        if ((rv = apr_bucket_read(b, &tempbuf, &len_read, APR_BLOCK_READ)) != APR_SUCCESS) { 
> -            return -1;
> -        }
> -        if (total + len_read > bufsiz) {
> -            apr_bucket_split(b, bufsiz - total);
> -            len_read = bufsiz - total;
> -        }
> -        memcpy(buffer, tempbuf, len_read);
> -        buffer += len_read;
> -        total += len_read;
> -        /* XXX the next two fields shouldn't be mucked with here, as they
> are in terms -         * of bytes in the unfiltered body; gotta see if
> anybody else actually uses -         * these
> -         */
> -        r->read_length += len_read;      /* XXX yank me? */
> -        old = b;
> -        b = APR_BUCKET_NEXT(b);
> -        apr_bucket_delete(old);
> -    }
> +    /* 1) Determine length to see if we may overrun.
> +     * 2) Partition the brigade at the appropriate point.
> +     * 3) Split the brigade at the new partition.
> +     * 4) Read the old brigade into the buffer.
> +     * 5) Destroy the old brigade.
> +     * 6) Point the context at the new brigade.
> +     */
> +    apr_brigade_length(bb, 1, &len);
> +
> +    if (bufsiz < len)
> +        len = bufsiz;
> +
> +    if (apr_brigade_partition(bb, len, &b) != APR_SUCCESS)
> +        return -1;
> +
> +    newbb = apr_brigade_split(bb, b);
> +
> +    if (apr_brigade_to_buffer(bb, buffer) != APR_SUCCESS)
> +        return -1;
> +
> +    if (apr_brigade_destroy(bb) != APR_SUCCESS)
> +        return -1;
> +
> +    req_cfg->bb = newbb;
>
> -    return total;
> +    return bufsiz;
>  }
>
>  /* In HTTP/1.1, any method can have a body.  However, most GET handlers

-- 

______________________________________________________________
Ryan Bloom				rbb@apache.org
Covalent Technologies			rbb@covalent.net
--------------------------------------------------------------