SLMT / telnet-rs

A simple implementation of Telnet in Rust.
MIT License
45 stars 20 forks source link

runtime switchable zlib stream. #6

Closed yehoshuapw closed 5 years ago

yehoshuapw commented 5 years ago

Added Ability - as compile time feature - to have a zlib stream wrap the Stream (including the telnet negotiations) at runtime.

This cannot be done using #2 's Stream, since the Stream needs to be switched at runtime - and it is moved into Telnet. (which only knows about it as Stream, and does not know how to switch it)

This is used for MCCP2=Compress2, which seems to be the only telnet negotiation which makes changes to the stream itself.

(Even as a compile time feature, not completely sure if this should be in main crate, or just stay as local change)

SLMT commented 5 years ago

Thanks for your afford! I will review your request ASAP.

SLMT commented 5 years ago

Hi @yehoshuapw . I just reviewed your code and have a question. So, this change is designed for a user who receives a TelnetOption that requests for Compress2. Then, the user can enable zlib streaming by simply calling telnet.begin_zlib(). Is that correct?

yehoshuapw commented 5 years ago

That is correct. Specifically MCCP2 calls for telnet negotiation of MCCP2, and then a short sub-negotiation. for example:

        match event {
            TelnetEvent::Data(buffer) => {
                println!("{}", &std::str::from_utf8(&(*buffer)).unwrap());
            },
            TelnetEvent::Negotiation(NegotiationAction::Will, TelnetOption::Compress2) => {
                telnet.negotiate(NegotiationAction::Do, TelnetOption::Compress2);
            },
            TelnetEvent::Subnegotiation(TelnetOption::Compress2, _) => {
                telnet.begin_zlib();
            }
        }

I have debated whatever it is possible to do with a less aggressive change, however:

SLMT commented 5 years ago

Ok. I think your code looks good for the current design, and it could be a feature of this crate. Would you mind adding an explanation about how to use MCCP2 (like your example code) and turn on the feature to the document of the crate?

yehoshuapw commented 5 years ago

Added some docs, which are on regardless of the feature's state. Having the feature on by default only for the docs will make many things look slightly different then they actually are - which is unclear to everyone who dosn't use the feature. (And having basic doc's explaining how to use it will show people it exists)