Opened 10 years ago

Closed 10 years ago

Last modified 19 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 10 years ago.
allow-protocol-overriding-progress.diff (3.0 KB) - added by Michael Hudson-Doyle 10 years ago.
allow-protocol-overriding-progress-2.diff (3.0 KB) - added by Michael Hudson-Doyle 10 years ago.
allow-protocol-overriding-progress-3.diff (3.0 KB) - added by Michael Hudson-Doyle 10 years ago.

Download all attachments as: .zip

Change History (19)

Changed 10 years ago by Michael Hudson-Doyle

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

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

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

Keywords: review added

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

Owner: z3p deleted

comment:4 Changed 10 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 10 years ago by Michael Hudson-Doyle

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

Keywords: review added
Owner: Michael Hudson-Doyle deleted

How does this look?

comment:6 Changed 10 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 10 years ago by Michael Hudson-Doyle

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

Keywords: review added
Owner: Michael Hudson-Doyle deleted

Now with less Launchpad coding style.

comment:8 Changed 10 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Jonathan Lange

OK. This is approved to land.

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

testBuildProtocol and testBuildProtocolRespectsProtocol are misnamed

comment:10 Changed 10 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 10 years ago by Michael Hudson-Doyle

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

Keywords: review added
Owner: Michael Hudson-Doyle deleted

Like so?

comment:12 Changed 10 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 10 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 7 years ago by <automation>

comment:15 Changed 19 months ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.