On Wed, Jun 17, 2020 at 10:43 AM Joe Orton <jor...@redhat.com> wrote: > > > 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.
At this point it isn't an HTTP/2 request IMHO, it's an HTTP/1 request extracted from an h2 stream. I suppose that using r->protocol = HTTP/2.0 is for (custom-)logging purpose, or is it specified? Stefan? > > 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 think we should not mark h2 to h1 requests using r->protocol, it's confusing whereas it somehow should be transparent for h1 core/modules. If something really needs to know, at worst r->proto_num could do, but I'd prefer a separate field or notes. If it's only for logging, possibly we could override that at a later stage/hook? As seen here, they really are not h2 requests as for h1 core/modules which actually handle them. > > 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. I'm not sure, what guarantees otherwise that the method and URI-path extracted from HTTP/2 (i.e. in h2_request_add_header) are valid? Do we check that elsewhere? > > 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? Besides r->protocol which we set to a known/valid value, we probably would have to duplicate all the other checks. I find it quite sane to reuse the same validation, unless I'm missing something like h2 to h1 requests are always valid (at this point)... Regards; Yann.
