Bypass #344
Bypass #344
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
==========================================
- Coverage 90.29% 89.89% -0.41%
==========================================
Files 60 60
Lines 3533 3551 +18
==========================================
+ Hits 3190 3192 +2
- Misses 343 359 +16 ☔ View full report in Codecov by Sentry. |
| @@ -117,16 +117,25 @@ def _handle_control_chars(self) -> None: | |||
| if not self.stdout: | |||
| raise ScrapliConnectionNotOpened | |||
|
|
|||
| if self._raw_buf.find(NULL) != -1: | |||
There was a problem hiding this comment.
I added this to preserve the old behavior. If I understand the rest of the code, this isn't needed since _read catches EOFError and should be able to be removed? (not c: would only evaluate to true if c == 0?)
|
hey again! this looks/feels sane but I prolly gotta look at it closer this weekend or something. all the telnet bits were ported from old std library telnet things in a reasonably faithful fashion so I'm a bit hesitant to change things -- or at least have to look closer since I for sure don't look at these parts too often! |
Yeah no rush. Just looking through the STD library version. The biggest difference that probably slows down this port is the size of the read. The STD library version does fairly small read of 50. While this one seems to grab everything at once. For commands that generate a lot of output, it will make any kind of non-linear inefficiencies are more impactful. Now I'm curious if the overhead I'm seeing would be less with a smaller read lol. https://github.com/python/cpython/blob/3.12/Lib/telnetlib.py#L527 I also notice that this port has a bug where it does not correctly interpret the escape sequence 0xFF 0xFF as a literal 0xFF. https://github.com/python/cpython/blob/3.12/Lib/telnetlib.py#L456 |
If buffers do not have any control characters then they can be processed in one bulk move rather than byte-by-byte. This is a lot more efficent with devices that never hit `_control_char_sent_limit`. Also the first call to `_handle_control_chars` in `read` is unnecessary since `_handle_control_chars` consumes the entire `_raw_buf`. (meaning it is always starts empty when you call `read`) Notable things not addressed: - Most instances of `b""` (a `bytes` structure) should be `bytearray()` since `bytes` are not mutable. - if a control sequence is not contained in a single read it will not get processed. - The following code would be a little bit more efficent as a `memoryview` since it would skip copying the contents of `_raw_buf` every iteration. `c, self._raw_buf = self._raw_buf[:1], self._raw_buf[1:]` I left those out to minimize this patch, but can circle back around and fix them later.
|
phew. fought with ssh2/python packaging a bit this morning then took a look at this! im all python-d out! but this all sounds reasonable to me! I just rebased this off main to make sure the test fixups worked (unrelated to this stuff), and to test things out and seems good. if you're still happy with this we can smack merge! |
Looks good to me. |
Description
Bypass buffers with no control chars.
If buffers do not have any control characters then they can be processed in one bulk move rather than byte-by-byte.
This is a lot more efficient with devices that never hit
_control_char_sent_limit.Also the first call to
_handle_control_charsinreadis unnecessary since_handle_control_charsconsumes the entire_raw_buf. (meaning it is always starts empty when you callread)Notable things not addressed:
b""(abytesstructure) should bebytearray()sincebytesare not mutable.not get processed.
memoryviewsince it would skip copying the contents of_raw_bufevery iteration.c, self._raw_buf = self._raw_buf[:1], self._raw_buf[1:]I left those out to minimize this patch, but can circle back around and fix them later.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I have tried the changes out locally with some of the scripts we have around.
Checklist:
make lintbeforecommitting!)
docstrings include a summary, args, returns and raises fields (even if N/A)
note if there are any considerations for the vrnetlab setup