Opened 9 years ago

Closed 9 years ago

Last modified 10 months ago

#3443 defect closed fixed (fixed)

twisted.conch.ssh.factory.SSHFactory.buildProtocol doesn't let you customize the protocol it creates

Reported by: Michael Hudson-Doyle Owned by:
Priority: normal Milestone:
Component: conch Keywords:
Cc: Branch: branches/ssh-factory-build-protocol-3443
branch-diff, diff-cov, branch-cov, buildbot
Author: jml

Description

We want to set tcp keepalives on all connections to our ssh server, which would seem to be most logically done in connectionMade of the protocol object created on a connection -- but there's no way to influence the class of the protocol object created here, other than copy-paste-hacking buildProtocol into your factory subclass.

If SSHFactory.buildProtocol delegated to Factory.buildProtocol, you could do it by defining 'protocol' in your factory subclass, as in most of the rest of Twisted.

Diff attached, no tests though.

Attachments (4)

allow-protocol-overriding.diff (1.0 KB) - added by Michael Hudson-Doyle 9 years ago.
allow-protocol-overriding-progress.diff (3.0 KB) - added by Michael Hudson-Doyle 9 years ago.
allow-protocol-overriding-progress-2.diff (3.0 KB) - added by Michael Hudson-Doyle 9 years ago.
allow-protocol-overriding-progress-3.diff (3.0 KB) - added by Michael Hudson-Doyle 9 years ago.

Download all attachments as: .zip

Change History (19)

Changed 9 years ago by Michael Hudson-Doyle

comment:1 Changed 9 years ago by Michael Hudson-Doyle

Component: coreconch
Owner: changed from Glyph to z3p
Type: enhancementdefect

comment:2 Changed 9 years ago by Michael Hudson-Doyle

Keywords: review added

comment:3 Changed 9 years ago by Michael Hudson-Doyle

Owner: z3p deleted

comment:4 Changed 9 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Michael Hudson-Doyle

If you want to test this (and you do), then probably the best thing to do is add a 'test_factory' file to t/conch/tests, and then demonstrate that calls to buildProtocol call self.protocol with the correct parameters.

It might even be ok to just check that the return value of buildProtocol is an instance of self.protocol.

Changed 9 years ago by Michael Hudson-Doyle

comment:5 Changed 9 years ago by Michael Hudson-Doyle

Keywords: review added
Owner: Michael Hudson-Doyle deleted

How does this look?

comment:6 Changed 9 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Michael Hudson-Doyle
  • Need two lines of vertical whitespace between methods
  • Use docstrings rather than comments for tests: """\nFoo.\n"""
  • Please add a docstring to testMultipleFactories.

Thanks so much.

Changed 9 years ago by Michael Hudson-Doyle

comment:7 Changed 9 years ago by Michael Hudson-Doyle

Keywords: review added
Owner: Michael Hudson-Doyle deleted

Now with less Launchpad coding style.

comment:8 Changed 9 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Jonathan Lange

OK. This is approved to land.

comment:9 Changed 9 years ago by Jean-Paul Calderone

testBuildProtocol and testBuildProtocolRespectsProtocol are misnamed

comment:10 Changed 9 years ago by Jonathan Lange

Owner: changed from Jonathan Lange to Michael Hudson-Doyle

Oops, missed that.

Specifically, they should be test_buildProtocol and test_buildProtocolRespectsProtocol.

Changed 9 years ago by Michael Hudson-Doyle

comment:11 Changed 9 years ago by Michael Hudson-Doyle

Keywords: review added
Owner: Michael Hudson-Doyle deleted

Like so?

comment:12 Changed 9 years ago by Jonathan Lange

author: jml
Branch: branches/ssh-factory-build-protocol-3443

(In [24899]) Branching to 'ssh-factory-build-protocol-3443'

comment:13 Changed 9 years ago by Jonathan Lange

Resolution: fixed
Status: newclosed

(In [24901]) Make SSHFactory use self.protocol to construct protocol instances.

  • Author: mwh
  • Reviewer: jml
  • Fixes #3443

Previously SSHFactory always instantiated an SSHServerTransport. This commit allows subclasses of SSHFactory to specify their own protocol -- usually a subclass of SSHServerTransport.

comment:14 Changed 6 years ago by <automation>

comment:15 Changed 10 months ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.