On 9/08/2014 4:57 a.m., Alex Rousskov wrote: > On 08/08/2014 09:48 AM, Amos Jeffries wrote: <snip> >> >> Audit results (part 1): >> >> * Please apply CharacterSet updates separately. >> >> * Please apply Tokenizer API updates separately. >> >> * please apply src/HttpHdrCc.h changes separately. > > Why? If the branch is merged, they will be applied with their own/ > existing commit messages. >
They are updates on previous feature changesets unrelated to FTP which will block future parser alterations (also unrelated to FTP) being backported if this project takes long to stabilize. If they have their own commit messages then cherrypicking out of the branch should be relatively easy. > >> * is FTP/1.1 really the protocol version of FTP parsed by Squid? (the >> AnyP::ProtocolVersion parameters) > ... >> - NP: 0,0 version should be usable and probably best since there does >> not appear to be any actual official version numbering for FTP. > > Since there is no "any actual official version numbering for FTP", we > are using what works best. Version 1.1 works best because it causes > fewer FTP exceptions in the general code because FTP control connections > are persistent by default, just like HTTP/1.1 connections. I think there > is a comment about that. > > Using 0.0 would probably create more exceptions. 0.0 will most likely > also screw up some ICAP services that expect /1.0 or /1.1 when parsing > headers. > > After reading the above arguments, do you still want us to switch to 0.0? > Yuck. Messy. Okay, leave as-is but please document that as the reason for using that version number and a TODO about cleaning up the message processing to stop assuming HTTP versions. > >> in src/cf.data.pre: >> * please document tproxy, name=, protocol=FTP as supported on ftp_port > > We do not know whether it is supported and do not plan on testing/fixing > that support right now. Do you still want us to document it as supported? > Yes. You have not changed the TcpAcceptor code for TPROXY lookups or the ACL code using port name. So they are supported. protocol= you explicitly added support in the parser. > >> in src/client_side.h: >> * what is "waiting for future HttpsServer home" meant to mean on >> postHttpsAccept() > > Unlike HTTP and FTP, there is currently no object dedicated to serving > HTTPS connections. HttpServer is misused for that on one occasion, but a > lot more work is needed to properly move HTTPS code from ConnStateData > into severs/HttpsServer.* That work is unrelated to FTP. Okay. Thanks. Amos
