input filters with mod_ext_filter [patch]

[Jeff Trawick]

(apologies for missing the right mail reference and nasty quoting... 
e-mail problems, and luckily I was browsing the archives on daedalus 
during my ISP-imposed silence)

Philip,

Thanks for submitting the patch.  I hope you will fix minor issues and 
resubmit for further review?

+ *
+ * Support for input filters added by Philipp Reisner in June 2003.
I don't know that there is a formal policy regarding this sort of 
attribution, but imagine what the source tree would look like if 
everybody did this.  That's what CHANGES is for.  (It won't affect 
whether or not I commit the final patch, but it is something to think 
about.)

+ *
+ * apxs2 -Wc,-Wall,-g -Wl,-g -c -i mod_ext_filter.c
axe

+static int ef_input_filter(ap_filter_t *f, apr_bucket_brigade *bb,
+                           ap_input_mode_t mode, apr_read_type_e block,
+                           apr_off_t readbytes);
shouldn't this forward decl go next to the one for ef_output_filter()?

-#if 0              /* no input filters yet */
+              /* no input filters yet */
uhh, that whole line needs to go

     else if (filter->mode == INPUT_FILTER) {
         /* XXX need a way to ensure uniqueness among all filters */
         ap_register_input_filter(filter->name, ef_input_filter, NULL, 
AP_FTYPE_RESOURCE);

why not filter->ftype instead of AP_FTYPE_RESOURCE?  all sorts of tricks 
can be done with ext output filters by allowing the astute user to 
override the default filter type

@@ -590,20 +599,24 @@
         return APR_EINVAL;
     }
     ctx->p = f->r->pool;
+
     if (ctx->filter->intype &&
patches are easier to review when there are no unnecessary changes... 
if you want to post another patch later for whitespace changes, that's 
fine but each patch should implement one feature/fix only

+       const char *ctypes = apr_table_get(ctx->filter->mode == 
INPUT_FILTER ?
+                                          f->r->headers_in : 
f->r->headers_out,
+                                          "Content-Type");
+       const char *ctype = ap_getword(f->r->pool, &ctypes, ';');
+

there are tab characters here and elsewhere...  you need to get rid of 
them (possibly use an editor setting to encode tabs as spaces)

+       ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_DEBUG, 0, f->r,
+                      "filter '%s': Content-Type='%s' => %s",
+                     ctx->filter->name, ctype,
+                     ctx->noop == 1 ? "noop" : "apply");
+
1) the trace doesn't make sense here since we haven't gotten to the last 
possible reason to no-op

2) APLOG_NOERRNO is for backwards compatibility only; you don't need it, 
since passing 0 in for the apr_status_t provides the required information

3) there is already a trace for this info and more anyway, at the end of 
the function

+ * if any data is available from the filter, read it and append it
+ * to the the bucket brigade
duplicate duplicate word

+static apr_status_t drain_available_output(ap_filter_t \ 
*f,apr_bucket_brigade *bb)

there should be a space before "apr_bucket_brigade" (apply comment globally)

@@ -716,7 +723,7 @@
              * to read data from the child process and pass it down to the
              * next filter!
              */
-            rv = drain_available_output(f);
+            rv = drain_available_output(f,bb);
there should be a space before "bb" (apply comment globally)

@@ -750,7 +757,14 @@
     return rv;
 }
-static apr_status_t ef_output_filter(ap_filter_t *f, apr_bucket_brigade 
*bb)
+/* ef_unified_filter:
+ *
+ * runs the bucket brigade bb through the filter and returns the
+ * and puts the result into bb. Dropping the previous content
+ * of bb (the input)

did you mean

"runs the bucket brigade bb through the filter and puts the result into 
bb, dropping the previous content of bb (the input)"

-        if (APR_BUCKET_IS_EOS(b)) {
+       if (APR_BUCKET_IS_EOS(b)) {
             eos = b;
             break;
         }
this is another hint that something changed in the patch that didn't need to

Thanks!