On Tue, Jun 16, 2020 at 12:08:41PM +0200, Ruediger Pluem wrote: > > > On 6/16/20 9:29 AM, Stefan Eissing wrote: > >> Am 15.06.2020 um 21:46 schrieb Ruediger Pluem <[email protected]>: > >> > >> I would like to unblock the test failure on trunk soon. Any comments on > >> the below? > > > > I am not really familiar with ap_parse_request_line() and why it was added > > there. Yann? > > > > As to "faking" the http version of a request, that does not look good. Do > > we need to preserve the fairy tale for our code that everything is HTTP/1.x? > > I think the point is that ap_parse_request_line is only designed to parse <= > HTTP/1.x requests, if it should parse >= HTTP/2.0 > I guess we need to adjust the API such that we can tell it which major and > probably minor version to consider. > But looking at the code again that brought me to the point that I would need > to reset r->the_request to > > r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0", req->method, > req->path ? req->path : ""); > > after the ap_parse_request_line() call. > And yes this makes me think if this kind of fake really makes sense. The need > to reset 3 request_rec fields after > ap_parse_request_line doesn't sound good. OTOH calling ap_parse_request_line > makes it possible to apply the same policies to > HTTP/2.0 and HTTP/1.x requests at least where they overlap. Maybe the change > to the API is the way out. > Keen to see Yann's feedback on this :-)
I'm not Yann but my 2c is that ap_parse_request_line() is designed to safely parse an HTTP/1.x request-line off the wire and probably shouldn't be used in mod_h2. The complexity of that function is dealing with all the error cases, which is not useful since none of them will be hit with HTTP/2. It looks cheaper - and not too complex - to set the fields of the request_rec directly as appropriate for HTTP/2 as ap_parse_request_line() does for HTTP/1.1? Regards, Joe
