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.
2) 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().)
3) 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.
|
➜
