Re: [PATCH] Native FTP Relay

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
Received on Fri Aug 08 2014 - 17:13:19 MDT