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
