Both the things you caught were probably just errors I made in manually
merging the patch into 2.2.x; here's a revised version, just in case.
Thanks for the review.
I'm not really clear on the error-notes discussion. It makes sense that we
wouldn't care too much if we don't save the ones from reading the headers,
since we would have errored earlier if they were set. What happens
currently if there's an error reading the trailers and we are not merging
the trailers? Seems like the error-notes would just get overwritten with
nothing, in the general case, and they wouldn't be able to be used anywhere.
On Sun, May 11, 2014 at 3:55 PM, Yann Ylavic ylavic@gmail.com wrote:
On Sun, May 11, 2014 at 6:35 PM, Yann Ylavic ylavic@gmail.com wrote:
Restoring the status and notes (set before the body is read) seems
unconditional to me.
Well, actually I'm not really sure whether we should preserve or
overwritte the error-notes here.
In most cases (but PROXYREQ_RESPONSE discussed below), we are still
reading the request.
If an error is raised by ap_get_mime_headers(), we probably want to
respond with a 4xx status and the corresponding error-notes (from this
call).
Moreover, the original error-notes (from the headers' parsing) is
very likely to be unset here (or we would have responded with that
error at that time).
However, when ap_http_filter() is called in PROXYREQ_RESPONSE mode, we
probably need to discard the forwarded response's trailers'
error-notes, since the client is not the culprit.
Maybe we should distinguish (at least) these 2 modes in the final patch.
Index: include/http_core.h
===
--- include/http_core.h (revision 1593540)
+++ include/http_core.h (working copy)
@@ -613,6 +613,10 @@
#define AP_TRACE_ENABLE1
#define AP_TRACE_EXTENDED 2
int trace_enable;
+#define AP_MERGE_TRAILERS_UNSET0
+#define AP_MERGE_TRAILERS_ENABLE 1
+#define AP_MERGE_TRAILERS_DISABLE 2
+int merge_trailers;
} core_server_config;
Index: include/httpd.h
===
--- include/httpd.h (revision 1593540)
+++ include/httpd.h (working copy)
@@ -1006,6 +1006,11 @@
* record to improve 64bit alignment the next time we need to break
* binary compatibility for some other reason.
*/
+
+/** MIME trailer environment from the request */
+apr_table_t *trailers_in;
+/** MIME trailer environment from the response */
+apr_table_t *trailers_out;
};
/**
Index: modules/http/http_filters.c
===
--- modules/http/http_filters.c (revision 1593540)
+++ modules/http/http_filters.c (working copy)
@@ -215,6 +215,7 @@
ap_input_mode_t mode, apr_read_type_e block,
apr_off_t readbytes)
{
+core_server_config *conf;
apr_bucket *e;
http_ctx_t *ctx = f-ctx;
apr_status_t rv;
@@ -222,6 +223,9 @@
int http_error = HTTP_REQUEST_ENTITY_TOO_LARGE;
apr_bucket_brigade *bb;
+conf = (core_server_config *)
+ap_get_module_config(f-r-server-module_config, core_module);
+
/* just get out of the way of things we don't want. */
if (mode != AP_MODE_READBYTES mode != AP_MODE_GETLINE) {
return ap_get_brigade(f-next, b, mode, block, readbytes);
@@ -403,13 +407,50 @@
}
if (!ctx-remaining) {
-/* Handle trailers by calling ap_get_mime_headers again! */
+int saved_status = f-r-status;
+apr_table_t *saved_headers_in = f-r-headers_in;
+const char *saved_error_notes = apr_table_get(f-r-notes,
+ error-notes);
+
+/* By default, don't merge in the trailers
+ * (treat UNSET as DISABLE) */
+if (conf-merge_trailers != AP_MERGE_TRAILERS_ENABLE) {
+f-r-status = HTTP_OK;
+f-r-headers_in = f-r-trailers_in;
+apr_table_clear(f-r-headers_in);
+}
+
ctx-state = BODY_NONE;
ap_get_mime_headers(f-r);
-e = apr_bucket_eos_create(f-c-bucket_alloc);
-APR_BRIGADE_INSERT_TAIL(b, e);
-ctx-eos_sent = 1;
-return APR_SUCCESS;
+if (f-r-status == HTTP_OK) {
+e = apr_bucket_eos_create(f-c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(b, e);
+ctx-eos_sent = 1;
+rv = APR_SUCCESS;
+}
+else {
+const char *error_notes = apr_table_get(f-r-notes,
+error-notes);
+ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f-r,
+ Error while reading HTTP
