On Mon, May 12, 2014 at 7:03 PM, Edward Lu <[email protected]> wrote:
> 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.
Looks good, but you did not address Eric's comment about parsing the
trailers directly into trailers_in and merge them later if required
(MergeTrailers on).
So imo :
f->r->status = HTTP_OK;
f->r->headers_in = f->r->trailers_in;
apr_table_clear(f->r->headers_in);
if (saved_error_notes) {
apr_table_unset(f->r->notes, "error-notes");
}
should be used unconditionnally before calling ap_get_mime_headers(),
be it only for the relevance of status == HTTP_OK or the error-notes'
log after the call.
> 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.
Agreed.
> 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.
If reading/parsing the trailers fails on the client side, we still
want to respond with the correct (trailers) error-notes (eg. trailer
too large, too many trailers...), so the error-notes should not be
unset/restored before leaving in this case.
However ap_http_filter() is also called for mod_proxy_http's backend
responses (r->proxyreq == PROXYREQ_RESPONSE), and in that case we
don't want the trailers' error-notes to polute anything, so it should
be unset/restored before leaving.
The same apply for r->status, the value set by ap_get_mime_headers()
is relevant when handling a request but not for a response (where the
one from the status line received earlier has to be preserved).
So r->status should be restored before leaving only in the
PROXYREQ_RESPONSE case.
