In addition to Ryan's comments I have the following comments...
Brian Pane wrote:
> How does this patch look? It does the check only on bucket
> boundaries, and it pushes out the buffered content if either:
> * the amount of buffered content is too large, or
> * the apr_bucket_read would block.
>
> If this technique looks sound, I can create another patch that
> does the same thing for find_end_sequence.
>
> --Brian
>
> Index: mod_include.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/modules/filters/mod_include.c,v
> retrieving revision 1.136
> diff -u -r1.136 mod_include.c
> --- mod_include.c 2001/08/27 20:25:42 1.136
> +++ mod_include.c 2001/08/28 06:16:38
> @@ -143,9 +143,10 @@
> * first byte of the BEGINNING_SEQUENCE (after finding a complete
> match) or it
> * returns NULL if no match found.
> */
> -static apr_bucket *find_start_sequence(apr_bucket *dptr, include_ctx_t
> *ctx,
> - apr_bucket_brigade *bb, int
> *do_cleanup)
> +static apr_bucket *find_start_sequence(apr_bucket *dptr, ap_filter_t *f,
> + apr_bucket_brigade **bb, int
> *do_cleanup)
> {
> + include_ctx_t *ctx = f->ctx;
> apr_size_t len;
> const char *c;
> const char *buf;
> @@ -156,39 +157,43 @@
> *do_cleanup = 0;
>
> do {
> + apr_status_t rv;
> +
> if (APR_BUCKET_IS_EOS(dptr)) {
> break;
> }
> - apr_bucket_read(dptr, &buf, &len, APR_BLOCK_READ);
> - /* XXX handle retcodes */
> - if (len == 0) { /* end of pipe? */
> - break;
> +
> + if ((ctx->bytes_parsed >= BYTE_COUNT_THRESHOLD) &&
> + !APR_BRIGADE_EMPTY(*bb)) {
> + apr_bucket_brigade *remainder = apr_brigade_split(*bb, dptr);
This should check ctx->head_start_bucket. If it is non-NULL, use it instead
of dptr in the brigade split or you may be passing part of the tag along.
> + rv = ap_pass_brigade(f->next, *bb);
> + *bb = remainder;
You need to reset ctx->bytes_parsed to 0 here or this will trip again.
> + if (rv != APR_SUCCESS) {
> + return NULL;
> + }
You may want to emulate the way the old code worked for threshold processing.
It split the bucket, but you don't *have* to do that. The important part is
what it did with the returned pointer and the context. It allowed the
code in send_parsed_content to do the actual pass_brigade. Thus the code
to split and pass the brigade existed in one common place for the
find_start_sequence and find_end_sequence code. So all you would have to
do here is reset the context and return a pointer to the head_start_bucket.
You would do the same thing in find_end_sequence. This would limit the
duplicated code to one set of code instead of four.
> }
> - c = buf;
> - while (c < buf + len) {
> - if (ctx->bytes_parsed >= BYTE_COUNT_THRESHOLD) {
> - apr_bucket *start_bucket;
>
> - if (ctx->head_start_index > 0) {
> - start_index = ctx->head_start_index;
> - start_bucket = ctx->head_start_bucket;
> + rv = apr_bucket_read(dptr, &buf, &len, APR_NONBLOCK_READ);
> + if (rv == APR_EAGAIN) {
> + if (!APR_BRIGADE_EMPTY(*bb)) {
> + apr_bucket_brigade *remainder = apr_brigade_split(*bb,
> dptr);
> + rv = ap_pass_brigade(f->next, *bb);
> + *bb = remainder;
Same comments here...
> + if (rv != APR_SUCCESS) {
> + return NULL;
> }
> - else {
> - start_index = (c - buf);
> - start_bucket = dptr;
> - }
> - apr_bucket_split(start_bucket, start_index);
> - tmp_bkt = APR_BUCKET_NEXT(start_bucket);
> - if (ctx->head_start_index > 0) {
> - ctx->head_start_index = 0;
> - ctx->head_start_bucket = tmp_bkt;
> - ctx->parse_pos = 0;
> - ctx->state = PRE_HEAD;
> - }
> -
> - return tmp_bkt;
> }
> + rv = apr_bucket_read(dptr, &buf, &len, APR_BLOCK_READ);
> + }
> + if (rv != APR_SUCCESS) {
> + return NULL;
> + }
>
> + if (len == 0) { /* end of pipe? */
> + break;
> + }
> + c = buf;
> + while (c < buf + len) {
> if (*c == str[ctx->parse_pos]) {
> if (ctx->state == PRE_HEAD) {
> ctx->state = PARSE_HEAD;
> @@ -244,7 +249,7 @@
> ctx->bytes_parsed++;
> }
> dptr = APR_BUCKET_NEXT(dptr);
> - } while (dptr != APR_BRIGADE_SENTINEL(bb));
> + } while (dptr != APR_BRIGADE_SENTINEL(*bb));
> return NULL;
> }
>
> @@ -2363,7 +2368,7 @@
> int do_cleanup = 0;
> apr_size_t cleanup_bytes = ctx->parse_pos;
>
> - tmp_dptr = find_start_sequence(dptr, ctx, *bb, &do_cleanup);
> + tmp_dptr = find_start_sequence(dptr, f, bb, &do_cleanup);
>
> /* The few bytes stored in the ssi_tag_brigade turned out
> not to
> * be a tag after all. This can only happen if the starting
