Issue 664020: telnetlib option subnegotiation fix (original) (raw)

The telnetlib patch #630829 (committed as v1.20) for option subnegotiation doesn't work as-is. There are several problems.

  1. The option negotiation callback should be a method of the Telnet class, not a separate function. The callback is supposed to call the read_sb_data method when it receives SB or SE, but of which object?

I think the callback should have been a method all along. It should be able to change the object's state based on the negotiation. Generally when implementing a new protocol extension, you'll subclass Telnet, add the new behavior, and then have the callback set a flag in the object to activate the new behavior after it has been negotiated.

The default DONT/WONT behavior can be moved out of the depths of process_rawq() into the callback method. Subclasses can then just forward to the parent class when they get an option they don't care about, instead of having to reimplement the default behavior.

If the callback is a method, the socket argument is still available as self.sock. The set_option_negotiation_callback method and the checks for "if self.option_callback:" would not be needed.

Changing the callback to a method will break compatibility with Python 2.2, however.

  1. On receipt of SB and SE, the callback is always called with NOOPT. The option is never passed. (It actually gets stored as the first part of the parameter data and returned by read_sb_data().)

  2. The callback is called on the receipt of both SB and SE. The SB call is completely superfluous. The option and parameter data have not been read yet, so there is nothing for the callback to do.

The attached patch fixes these and adds support for the NOP command. It also includes documentation changes and an example of implementing some options.

Some other thoughts:

The name "option_callback" is a little misleading. The callback is executed in response to all telnet protocol commands, not just the ones dealing with options.

Why define and pass NOOPT for the case when there is no option rather than use None?

The only place the SB parameter data will (can safely?) be used is in the callback. Why not pass it as another optional parameter and skip the need for read_sb_data()?

A potential name conflict: the EOR telnet option defines a new command code also called EOR (with a much different numeric value). The C header file <arpa/telnet.h> prefixes all the options with TELOPT_, so there is no conflict there, but telnetlib drops the prefix.