On Sun, Sep 23, 2001 at 06:58:19PM -0700, Justin Erenkrantz wrote:
>...
> --- server/protocol.c 2001/09/18 22:13:57 1.45
> +++ server/protocol.c 2001/09/24 01:45:18
>...
> @@ -562,6 +562,9 @@
> r->notes = apr_table_make(r->pool, 5);
>
> r->request_config = ap_create_request_config(r->pool);
> + /* Must be set before we run create request hook */
> + r->output_filters = conn->output_filters;
> + r->input_filters = conn->input_filters;
> ap_run_create_request(r);
> r->per_dir_config = r->server->lookup_defaults;
>
> @@ -572,8 +575,6 @@
>
> r->status = HTTP_REQUEST_TIME_OUT; /* Until we get a request */
> r->the_request = NULL;
> - r->output_filters = conn->output_filters;
> - r->input_filters = conn->input_filters;
Let's go ahead and keep this change, even though we aren't turning HTTP_IN
into a request filter.
>...
> --- modules/http/http_protocol.c 2001/09/17 13:12:37 1.362
> +++ modules/http/http_protocol.c 2001/09/24 01:45:18
> @@ -481,125 +481,115 @@
> return AP_HTTP_METHODS[methnum];
> }
>
> -struct dechunk_ctx {
> - apr_size_t chunk_size;
> - apr_size_t bytes_delivered;
> +static long get_chunk_size(char *);
> +
> +typedef struct http_filter_ctx {
> + apr_bucket_brigade *b;
> + int status;
The whole mess with this "status" field is a hack. There should be comments
inserted to that effect. Not your fault, and we have to deal with it for a
bit, but it should at least be noted.
Ideally, when this filter becomes a request filter (later), we can simply
delay adding it until the higher level has processed the headers. Once the
headers are out of the way, then this filter can be added to process the
message body.
(i.e. move its insertion to the point you mention: line 626 of protocol.c )
> + apr_size_t remaining;
> enum {
> - WANT_HDR /* must have value zero */,
> - WANT_BODY,
> - WANT_TRL
> + BODY_NONE /* must have value of zero */,
It doesn't have zero if you simply test for this state. i.e. rather than
saying "if (ctx->state)" .. just say "if (ctx->state != BODY_NONE)". It
could also improve clarity.
> + BODY_LENGTH,
> + BODY_CHUNK
> } state;
> -};
> -
> -static long get_chunk_size(char *);
> +} http_ctx_t;
>
> -apr_status_t ap_dechunk_filter(ap_filter_t *f, apr_bucket_brigade *bb,
> - ap_input_mode_t mode, apr_off_t *readbytes)
> +/* Hi, I'm the main input filter for HTTP requests.
> + * I handle chunked and content-length bodies. */
> +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 rv;
> - struct dechunk_ctx *ctx = f->ctx;
> - apr_bucket *b;
> - const char *buf;
> + apr_bucket *e;
> + char *buff;
> apr_size_t len;
> + char *pos;
> + http_ctx_t *ctx = f->ctx;
> + apr_status_t rv;
>
> if (!ctx) {
> - f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(struct dechunk_ctx));
> + f->ctx = ctx = apr_pcalloc(f->r->pool, sizeof(*ctx));
> + ctx->b = apr_brigade_create(f->r->pool);
> + ctx->status = f->r->status;
> +
> + /* Due to the way the core is designed, this should happen each time
> + * we get initialized. */
> + if ((rv = ap_get_brigade(f->next, ctx->b, mode,
> + readbytes)) != APR_SUCCESS) {
> + return rv;
> + }
> }
>
> - 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.
> - */
> - char line[30];
> + /* Basically, we have to stay out of the way until server/protocol.c
> + * says it is okay - which it does by setting r->status to OK. */
> + if (f->r->status != ctx->status)
> + {
> + int old_status;
> + /* Allow us to be reentrant! */
> + old_status = ctx->status;
> + ctx->status = f->r->status;
> +
> + if (old_status == HTTP_REQUEST_TIME_OUT && f->r->status == HTTP_OK)
There should be a comment that HTTP_REQUEST_TIME_OUT is the initial value
for r->status. Therefore, this code section is effectively an init()
sequence for processing the message body.
> + {
> + const char *tenc, *lenp;
> + tenc = apr_table_get(f->r->headers_in, "Transfer-Encoding");
> + lenp = apr_table_get(f->r->headers_in, "Content-Length");
> +
> + if (tenc) {
> + if (!strcasecmp(tenc, "chunked")) {
> + char line[30];
>
>...
> + if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
> + return rv;
> + }
> + ctx->state = BODY_CHUNK;
> + ctx->remaining = get_chunk_size(line);
The above section of code, and some of the stuff following is duplicated
from ap_setup_client_block(). There should be a comment about this
duplicated code. Especially because this copy doesn't contain all of the
error checking and responses of the ap_setup_client_block version. A next
step would be to figure out how to remove that duplication.
Specifically, I'd think that we figure out some of this information when we
insert the [request form of this] filter. The ap_read_request() function
would set up the filter context and set it when the filter is added; the
context would indicate the particular parameters for body reading.
[ maybe at some point in the future, the filter can read the request line,
headers, body, etc, and do all of the computation and checking itself ]
>...
> + else if (ctx->state == BODY_CHUNK)
> + {
> + char line[30];
> +
> + ctx->state = BODY_NONE;
>...
> + /* We need to read the CRLF after the chunk. */
> + if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
> + return rv;
> + }
Is the BODY_NONE thing so that the ap_getline() recursion thingy works
properly, and returns a line?
>...
> + /* This had better be equal to zero now! */
> + if (ctx->status)
> + ctx->remaining -= total;
What better be zero? status or remaining?
I'm not sure why ctx->status would ever be zero. Could you explain? Is that
at some point where r->status becomes OK (rather than HTTP_OK). A lot of
stuff seems to key off ctx->status != 0. I don't see why...
>...
> AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, apr_size_t
>bufsiz)
Probably just leave this be, since we don't have the apr_brigade_to_buffer
function for now.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
