Passive port ranges are not settable in t.p.ftp

[krp]

As discussed on #twisted, FTP passive ports are chosed randomly by the OS (since we use reactor.listenTCP(0, f) for the dtp factory).

I propose this patch that offers the user the possibility to tune the passive port ranges.

To be reviewed, it may not fit twisted's style.

[therve]

This is ready to review in ftp-pasv-port-range-1904.

spiv, willing to have a look?

[exarkun]

So, here is a crazy thought

Port ranges should be determined by some method on the avatar (perhaps not a method which is part of the primary interface, though, since that seems to be oriented towards filesystem interaction, although maybe it'd be okay to add it there anyway).

[therve]

Replying to exarkun:

So, here is a crazy thought Port ranges should be determined by some method on the avatar (perhaps not a method which is part of the primary interface, though, since that seems to be oriented towards filesystem interaction, although maybe it'd be okay to add it there anyway).

It seems it's totally possible, but what does it bring exactly? We'll move the information of the ports from FTPFactory/FTP to FTPRealm/FTPShell. It looks relatively equivalent to me. If your point is for customisation, I've never seen a case where you want port settings customizable by client, but that could be useful for someone.

[exarkun]

Hmm, you're right, it's a pretty rare use-case. Okay, let's forget about that for now.

What I would like to see, then, is some simpler semantics for the range. How about this, instead of having a min and a max (with 0 for min having a special meaning), there is a single attribute which gives a port range. It can just be an iterator (eg, xrange(10, 20)). getDtpPort can just iterate over it. This should be less code and offer more flexibility.

How does this sound?

Incidentally, getDtpPort should capitalize DTP, since it's an acronym (Data Transfer Protocol).

[therve]

(In [20940]) Use a range for passive port instead of 2 variables.

Refs #1904

[therve]

This sounds good :).

[exarkun]

Looking good, thanks!

I'd suggest some minor adjustments to the tests - I don't think any of them need to bind actual tcp ports. Two of them are careful not to, but test_PASVportRange seems to want to, and I'm not sure why. It could easily provide a listenFactory that asserts the beginning of the range is passed in first, and so on. Also, it has a weird name :) It might be okay to expand "pasv" to "passive" (maybe in the attribute name on FTP, too).

If test_PASVportRange stops using the real reactor.listenTCP, then it probably also won't have to take care to return the deferred returned by port.stopListening to avoid a cleanup error.

[therve]

(In [21114])

• Remove test using listenTCP
• Rename pasvPortRange to passivePortRange

Refs #1904.

[therve]

This test actually acts as a duplicate of test_portRange, so I removed it, and I rename the variable.

[exarkun]

Using list() on passivePortRange when raising CannotListenError from getDTPPort worries me a little. I might not want to see a list of 5 thousand port numbers if that exception gets logged. Maybe just having passivePortRange there would be okay? xrange objects str kind of nicely on their own, at least.

test_portRangeForwardError still sets pasvMinPort which probably doesn't do anything anymore.

Some suggestions for docstrings:

• test_portRangeForwardError - Exceptions other than L{error.CannotListenError} which are raised by C{listenFactory} should be raised to the caller of L{FTP.getDTPPort}.
• test_portRange - L{FTP.passivePortRange} should determine the ports which L{FTP.getDTPPort} attempts to bind. If no port from that iterator can be bound, L{error.CannotListenError} should be raised, otherwise the first successful result from L{FTP.listenFactory} should be returned.
• FTP.listenFactory - @ivar listenFactory: A callable with the signature of L{twisted.internet.interfaces.IReactorTCP.listenTCP} which will be used to create Ports for passive connections (mainly for testing).

These are all quite minor points. I think you should be able to merge without another review.

[therve]

(In [21189]) Apply review comments.

Refs #1904

[therve]

(In [21190]) Merge ftp-pasv-port-range-1904

Author: therve Reviewer: exarkun Fixes #1904

Throught the passivePortRange attribute of twisted.protocols.ftp.FTPFactory, allow a FTP server to restrict the passive ports used instead of using a random port.