Re: /bzr/squid3/trunk/ r13384: Bug 1961: pt1: URL handling redesign

On 3/05/2014 3:13 a.m., Alex Rousskov wrote:
> 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.
>

Is there time to get that converted? it would resolve a number of those
issues about adapted messages not having things like external ACL
tag/log carried across.

>
>> 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.

Altered the default Http::ProtocolVersion constructor. eCAP/ICAP should
be getting at least relatively acurate version details now. Modulo the
reply parsers output.

Amos
Received on Sat May 03 2014 - 13:33:46 MDT