You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Rici Lake <ri...@ricilake.net> on 2005/04/25 23:09:05 UTC

For review: teach mod_include.c to recycle brigades

Index: mod_include.c
===================================================================
--- mod_include.c       (revision 158730)
+++ mod_include.c       (working copy)
@@ -3060,7 +3060,7 @@
      struct ssi_internal_ctx *intern = ctx->intern;
      request_rec *r = f->r;
      apr_bucket *b = APR_BRIGADE_FIRST(bb);
-    apr_bucket_brigade *pass_bb;
+    apr_bucket_brigade *pass_bb = ctx->pass_bb;
      apr_status_t rv = APR_SUCCESS;
      char *magic; /* magic pointer for sentinel use */

@@ -3076,9 +3076,6 @@
          return ap_pass_brigade(f->next, bb);
      }

-    /* All stuff passed along has to be put into that brigade */
-    pass_bb = apr_brigade_create(ctx->pool, f->c->bucket_alloc);
-
      /* initialization for this loop */
      intern->bytes_read = 0;
      intern->error = 0;
@@ -3145,7 +3142,6 @@
              if (!APR_BRIGADE_EMPTY(pass_bb)) {
                  rv = ap_pass_brigade(f->next, pass_bb);
                  if (rv != APR_SUCCESS) {
-                    apr_brigade_destroy(pass_bb);
                      return rv;
                  }
              }
@@ -3170,7 +3166,7 @@
              }

              if (rv != APR_SUCCESS) {
-                apr_brigade_destroy(pass_bb);
+                apr_brigade_cleanup(pass_bb);
                  return rv;
              }

@@ -3384,7 +3380,7 @@
                      DEBUG_INIT(ctx, f, pass_bb);
                      rv = handle_func(ctx, f, pass_bb);
                      if (rv != APR_SUCCESS) {
-                        apr_brigade_destroy(pass_bb);
+                        apr_brigade_cleanup(pass_bb);
                          return rv;
                      }
                  }
@@ -3450,13 +3446,11 @@

      /* if something's left over, pass it along */
      if (!APR_BRIGADE_EMPTY(pass_bb)) {
-        rv = ap_pass_brigade(f->next, pass_bb);
+        return ap_pass_brigade(f->next, pass_bb);
      }
      else {
-        rv = APR_SUCCESS;
-        apr_brigade_destroy(pass_bb);
+        return APR_SUCCESS;
      }
-    return rv;
  }


@@ -3542,8 +3536,11 @@
          intern->end_seq_len = strlen(intern->end_seq);
          intern->undefined_echo = conf->undefined_echo;
          intern->undefined_echo_len = strlen(conf->undefined_echo);
+
+        /* Initialize the pass brigade */
+        ctx->pass_bb = apr_brigade_create(f->r->pool, 
f->c->bucket_alloc);
      }

      if ((parent = ap_get_module_config(r->request_config, 
&include_module))) {
          /* Kludge --- for nested includes, we want to keep the 
subprocess
           * environment of the base document (for compatibility); that 
means
@@ -3599,6 +3596,6 @@
          apr_table_setn(r->subprocess_env, "QUERY_STRING_UNESCAPED",
                    ap_escape_shell_cmd(r->pool, arg_copy));
      }

      return send_parsed_content(f, b);
  }
Index: mod_include.h
===================================================================
--- mod_include.h       (revision 158730)
+++ mod_include.h       (working copy)
@@ -85,6 +85,9 @@
      /* currently configured time format */
      const char  *time_str;

+    /* bucket brigade for passing to next filter */
+    apr_bucket_brigade *pass_bb;
+
      /* pointer to internal (non-public) data, don't touch */
      struct ssi_internal_ctx *intern;
  } include_ctx_t;


Re: For review: teach mod_include.c to recycle brigades

Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Monday, April 25, 2005 4:47 PM -0500 Rici Lake <ri...@ricilake.net> 
wrote:

> explicit cleanup, while we continue thrashing out that issue. (nd -- do
> you have a test suite, by any chance?)

httpd-test's has a bunch of tests for mod_include.  -- justin

Re: For review: teach mod_include.c to recycle brigades

Posted by Rici Lake <ri...@ricilake.net>.
On 25-Apr-05, at 4:27 PM, André Malo wrote:

> * Rici Lake wrote:
>
> +1 in general, but ...
>
>
> ...put it into the internal structure, please. That's I've created it 
> for
> (binary compat and only making stuff public what matters the public).
>
> nd
> -- 
>>>> Muschelflucht-Zusatzeinrichtung.
>>> Shell-Escape ist ja noch klar, aber `Zusatzeinrichtung'?
>> extension?
> Feature.                          -- gefunden in de.org.ccc


Oops, sorry. Revised patch, includes both the bug fix and the recycling 
of bucket brigades, and still insufficiently tested. Also includes an 
explicit cleanup, while we continue thrashing out that issue. (nd -- do 
you have a test suite, by any chance?)

Index: mod_include.c
===================================================================
--- mod_include.c       (revision 158730)
+++ mod_include.c       (working copy)
@@ -173,6 +173,7 @@
      apr_size_t    bytes_read;

      apr_bucket_brigade *tmp_bb;
+    apr_bucket_brigade *pass_bb;

      request_rec  *r;
      const char   *start_seq;
@@ -3060,7 +3061,7 @@
      struct ssi_internal_ctx *intern = ctx->intern;
      request_rec *r = f->r;
      apr_bucket *b = APR_BRIGADE_FIRST(bb);
-    apr_bucket_brigade *pass_bb;
+    apr_bucket_brigade *pass_bb = intern->pass_bb;
      apr_status_t rv = APR_SUCCESS;
      char *magic; /* magic pointer for sentinel use */

@@ -3076,9 +3077,6 @@
          return ap_pass_brigade(f->next, bb);
      }

-    /* All stuff passed along has to be put into that brigade */
-    pass_bb = apr_brigade_create(ctx->pool, f->c->bucket_alloc);
-
      /* initialization for this loop */
      intern->bytes_read = 0;
      intern->error = 0;
@@ -3145,7 +3143,6 @@
              if (!APR_BRIGADE_EMPTY(pass_bb)) {
                  rv = ap_pass_brigade(f->next, pass_bb);
                  if (rv != APR_SUCCESS) {
-                    apr_brigade_destroy(pass_bb);
                      return rv;
                  }
              }
@@ -3170,7 +3167,7 @@
              }

              if (rv != APR_SUCCESS) {
-                apr_brigade_destroy(pass_bb);
+                apr_brigade_cleanup(pass_bb);
                  return rv;
              }

@@ -3384,7 +3381,7 @@
                      DEBUG_INIT(ctx, f, pass_bb);
                      rv = handle_func(ctx, f, pass_bb);
                      if (rv != APR_SUCCESS) {
-                        apr_brigade_destroy(pass_bb);
+                        apr_brigade_cleanup(pass_bb);
                          return rv;
                      }
                  }
@@ -3450,13 +3447,14 @@

      /* if something's left over, pass it along */
      if (!APR_BRIGADE_EMPTY(pass_bb)) {
+        /* return ap_pass_brigade(f->next, pass_bb); */
          rv = ap_pass_brigade(f->next, pass_bb);
+        apr_brigade_cleanup(pass_bb);
+        return rv;
      }
      else {
-        rv = APR_SUCCESS;
-        apr_brigade_destroy(pass_bb);
+        return APR_SUCCESS;
      }
-    return rv;
  }


@@ -3542,8 +3540,10 @@
          intern->end_seq_len = strlen(intern->end_seq);
          intern->undefined_echo = conf->undefined_echo;
          intern->undefined_echo_len = strlen(conf->undefined_echo);
-    }

+        /* Initialize the pass brigade */
+        intern->pass_bb = apr_brigade_create(f->r->pool, 
f->c->bucket_alloc);
+
      if ((parent = ap_get_module_config(r->request_config, 
&include_module))) {
          /* Kludge --- for nested includes, we want to keep the 
subprocess
           * environment of the base document (for compatibility); that 
means
@@ -3599,6 +3599,7 @@
          apr_table_setn(r->subprocess_env, "QUERY_STRING_UNESCAPED",
                    ap_escape_shell_cmd(r->pool, arg_copy));
      }
+    }

      return send_parsed_content(f, b);
  }


Re: For review: teach mod_include.c to recycle brigades

Posted by André Malo <nd...@perlig.de>.
* Rici Lake wrote:

+1 in general, but ...

> Index: mod_include.h
> ===================================================================
> --- mod_include.h       (revision 158730)
> +++ mod_include.h       (working copy)
> @@ -85,6 +85,9 @@
>       /* currently configured time format */
>       const char  *time_str;
>
> +    /* bucket brigade for passing to next filter */
> +    apr_bucket_brigade *pass_bb;
> +
>       /* pointer to internal (non-public) data, don't touch */
>       struct ssi_internal_ctx *intern;
>   } include_ctx_t;

...put it into the internal structure, please. That's I've created it for 
(binary compat and only making stuff public what matters the public).

nd
-- 
>>> Muschelflucht-Zusatzeinrichtung.
>> Shell-Escape ist ja noch klar, aber `Zusatzeinrichtung'?
> extension?
Feature.                          -- gefunden in de.org.ccc