telnet-rs icon indicating copy to clipboard operation
telnet-rs copied to clipboard

Separate I/O from Telnet Parsing & Formatting

Open JamesStewy opened this issue 3 years ago • 4 comments

Following on from #15, this PR separates I/O from the parsing and formatting of Telnet data to allow alternate I/O models such as async/await.

TODO:

  • [x] Parser & Formatting functions
  • [x] Synchronous API with split reads and writes
  • [ ] Compression in Synchronous API
  • [x] No copy writes for data
  • [ ] Tests
  • [ ] Docs & README

JamesStewy avatar Mar 18 '21 12:03 JamesStewy

Following on from the discussion in the issue:

I am thinking maybe it means that we should not implement Write for the telnet writer. Since the telnet protocol is an application layer protocol, it is inevitable to add some other bytes when writing data. I check a few other libraries such as hyper. They also do not implement Write for their high level write APIs. I believe it is because they are treating the user data as 'objects' instead of 'bytes' so they do not need to implement Write to handle 'bytes' for users. However, if you thought it would be better to implement Write for the telnet writer, we could first check if there is any other library encountering the same issue like us.

I also couldn't find any examples of this being done in other libraries and agree that the Write trait isn't suitable. I have removed it from this PR.

One issue I have with the current proposal is that data is copied into a box as a part of formatting (escaping) it. It is possible and would be faster if data formatting was done entirely through slices. One way this could be done is changing format::data to pub fn data(data: &[u8]) -> Vec<&[u8]>. You would then use it like this:

let data = [1, 2, 0xFF, 3, 4];
let buffer = telnet::format::data(&data);
for b in buffer.into_iter() {
    stream.write_all(b)?;
}

Thoughts?

JamesStewy avatar Mar 18 '21 13:03 JamesStewy

Sure, I agree with you. We should use slicing instead of copying for the data. Your proposal for format::data looks good to me.

SLMT avatar Mar 19 '21 13:03 SLMT

I have made the return type for format::data a new type to allow the inclusion of a to_owned() method which copies the formatted data slices into a Box.

JamesStewy avatar Mar 20 '21 10:03 JamesStewy

Any updates?

lambdalisue avatar Oct 14 '21 12:10 lambdalisue