On 05/01/2014 07:36 AM, Amos Jeffries wrote:
> On 1/05/2014 10:21 p.m., Tsantilas Christos wrote:
>> If I am not wrong, currently the protocol in HttpMsg::http_ver is
>> always AnyP::PROTO_HTTP.
>> I think this is normal because we are always handling HTTP messages.
> You are right. eCap not using the version min.maj values should mean we
> dont have to worry about incorrect versions for this fix.
libecap::FirstLine::version() is supposed to provide the version so we
do need to worry about it.
> * Adaptation::Icap::ModXact::prepEchoing() does not copy the http_ver
> value. It appears to parse only the mime headers portion of the ICAP output.
> - I belive this means it is setting the adapted request/reply to
> "HTTP/0.0" default and ignoring any request-line/status-line changes the
> ICAP service tries to make.
prepEchoing() is for cases where there is no adapted request/reply and
Squid ICAP code has to "echo" the virgin message back to Squid core.
prepEchoing() should clone() instead of parsing(), but it was written
before cloning become viable in this context. The ICAP code calls
HttMsg::parse() which, AFAICT, does parse the first line of the message.
If I am wrong, a short-term fix would be to copy the http_ver manually
after we parse, I guess.
> TODO: http_ver is not set when created by any call to urlParse() which
> generates a new request object. This needs checking whether the callers
> set it.
>
> TODO: I have not yet tracked down exactly what code is doing the
> first-line parse for the encapsulated ICAP requests or replies. So am
> not certain the cloned value is being overwritten properly with ICAP
> sent values.
> -> Is prepEchoing() behaviour a hint that there is no such code?
Is not that the code in parseHead() called from parseHttpHead()?
parseHead() calls HttpMsg::parse(). HttpMsg::parse() parses the whole
message header, including the first line, right?
> I am thinking at this point that we should change the constructor on new
> HttpRequest/HttpReply to set default of HTTP/1.1 matching Squids
> official version. Then only touch http_ver when cloning/copying objects
> or parsing external inputs.
Sounds like a good idea to me. Alternatively, we could make the
protocol/version a required parameter, to help us track cases where it
is left unset, but I am guessing you have already identified all or
nearly all important cases, so defaulting to HTTP/1.1 is probably a
better approach.
Thank you,
Alex.
Received on Fri May 02 2014 - 15:14:06 MDT
This archive was generated by hypermail 2.2.0 : Sat May 03 2014 - 12:00:10 MDT