For Review: mod_ext_filter.c

[Rici Lake <ri...@ricilake.net>]

Patch to use a single temporary brigade instead of creating a new one 
on each invocation:
(also does a little code reuse)

Index: mod_ext_filter.c
===================================================================
--- mod_ext_filter.c    (revision 158730)
+++ mod_ext_filter.c    (working copy)
@@ -70,6 +70,7 @@
  #if APR_FILES_AS_SOCKETS
      apr_pollset_t *pollset;
  #endif
+    apr_bucket_brigade *bb_tmp;
  } ef_ctx_t;

  module AP_MODULE_DECLARE_DATA ext_filter_module;
@@ -614,6 +615,12 @@
          }
      }

+    /*
+     * Create a temporary brigade for output filter. This is not always
+     * used, but it's easier to just have it.
+     */
+    ctx->bb_tmp = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
+
      if (dc->debug >= DBGLVL_SHOWOPTIONS) {
          ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, f->r,
                        "%sfiltering `%s' of type `%s' through `%s', cfg 
%s",
@@ -747,10 +754,8 @@
      apr_status_t rv;
      char buf[4096];
      apr_bucket *eos = NULL;
-    apr_bucket_brigade *bb_tmp;

      dc = ctx->dc;
-    bb_tmp = apr_brigade_create(r->pool, c->bucket_alloc);

      for (b = APR_BRIGADE_FIRST(bb);
           b != APR_BRIGADE_SENTINEL(bb);
@@ -769,15 +774,14 @@

          /* Good cast, we just tested len isn't negative */
          if (len > 0 &&
-            (rv = pass_data_to_filter(f, data, (apr_size_t)len, 
bb_tmp))
+            (rv = pass_data_to_filter(f, data, (apr_size_t)len, 
ctx->bb_tmp))
                  != APR_SUCCESS) {
              return rv;
          }
      }

      apr_brigade_cleanup(bb);
-    APR_BRIGADE_CONCAT(bb, bb_tmp);
-    apr_brigade_destroy(bb_tmp);
+    APR_BRIGADE_CONCAT(bb, ctx->bb_tmp);

      if (eos) {
          /* close the child's stdin to signal that no more data is 
coming;
@@ -800,30 +804,12 @@
          }
      }

-    do {
-        len = sizeof(buf);
-        rv = apr_file_read(ctx->proc->out,
-                      buf,
-                      &len);
-        if ((rv && !APR_STATUS_IS_EOF(rv) && 
!APR_STATUS_IS_EAGAIN(rv)) ||
-            dc->debug >= DBGLVL_GORY) {
-            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r,
-                          "apr_file_read(child output), len %" 
APR_SIZE_T_FMT,
-                          !rv ? len : -1);
-        }
-        if (APR_STATUS_IS_EAGAIN(rv)) {
-            if (eos) {
-                /* should not occur, because we have an APR timeout in 
place */
-                AP_DEBUG_ASSERT(1 != 1);
-            }
-            return APR_SUCCESS;
-        }
-
-        if (rv == APR_SUCCESS) {
-            b = apr_bucket_heap_create(buf, len, NULL, 
c->bucket_alloc);
-            APR_BRIGADE_INSERT_TAIL(bb, b);
-        }
-    } while (rv == APR_SUCCESS);
+    rv = drain_available_output(f, bb);
+    if (APR_STATUS_IS_EAGAIN(rv)) {
+        AP_DEBUG_ASSERT(!eos);
+        /* should not occur, because we have an APR timeout in place */
+        return APR_SUCCESS;
+    }

      if (!APR_STATUS_IS_EOF(rv)) {
          return rv;