Add EOL translation when sending and receiving text not in BINARY mode #32
Conversation
|
Alternatively, it would be possible to merge the telnet_translate_eol() functionality into _process() and have it be enabled by a bit in the flags byte for telnet_init(). |
libtelnet.c
Outdated
| return size; | ||
|
|
||
| /* fix up translation if split across buffer boundary */ | ||
| if (*split && size > 0) { |
There was a problem hiding this comment.
both here and below on line ~1171, I think I'd prefer using i < size instead of size > 0, just because that makes it more directly obvious why the code needs the check at all (because buffer[i] is used).
libtelnet.c
Outdated
| @@ -1121,6 +1137,47 @@ static void _process(telnet_t *telnet, const char *buffer, size_t size) { | |||
| } | |||
| } | |||
|
|
|||
| /* perform NVT end-of-line translation for received text */ | |||
| size_t telnet_translate_eol(telnet_t *telnet, char *buffer, size_t size, | |||
| int *split) { | |||
There was a problem hiding this comment.
split could use a more descriptive name.
libtelnet.c
Outdated
| } else | ||
| *split = 0; | ||
| } | ||
| return size - (i - l); |
There was a problem hiding this comment.
If I can't tell if then return value is correct with a glance (and I can't) it probably needs some refactoring, better variable names, or so on. One approach may be to just modify size or a copy there-of rather than trying to recompute it.
There was a problem hiding this comment.
I made this harder than it needs to be. How about:
- At the top of the function, if (size == 0) return size;
2: Change names i -> in and l -> trans
3: At the end, just return trans + 1;
There was a problem hiding this comment.
[In my defense, please note that my use of i and l was just following the pattern of the code in telnet_send()]
libtelnet.c
Outdated
| if (buffer[i] == '\r') { | ||
| if (buffer[i+1] == '\n') { | ||
| /* CR-LF translates to \n */ | ||
| buffer[i++] = '\n'; |
There was a problem hiding this comment.
This line looks wrong. Should it not be modifying buffer[l] (lowercase L) instead of buffer[i++]? it looks like this would break on inputs with multiple \r\n sequences.
libtelnet.c
Outdated
| (buffer[i] == '\r' || buffer[i] == '\n')) { | ||
| /* dump prior portion of text */ | ||
| if (i != l) | ||
| _send(telnet, buffer + l, i - l); |
There was a problem hiding this comment.
The copy of this check right above has braces around the statement but this one does not.
There was a problem hiding this comment.
OK. I generally prefer to omit braces where they are not needed, but try to follow conventions in other people's code. I noted that there are statement both with and without in libtelnet. My guess as to how these two cases became inconsistent is that I copied 1379 with the braces but then later removed them after writing the next if statement at 1384 without braces and then noticing that 1379 was inconsistent with 1384. I'd be happy to put braces on both.
libtelnet.c
Outdated
| if (buffer[i] == '\r') | ||
| _send(telnet, CRNUL, 2); | ||
| /* automatic translation of \n -> CRLF */ | ||
| else _send(telnet, CRLF, 2); |
There was a problem hiding this comment.
Not a fan of folding the statement onto the same line as the else.
There was a problem hiding this comment.
I am a fan, but I know others are not, so before using it I looked to see that there were several other instances already in the file.
Thanks for making (beginning?) your review. I am about to head out the door, so I will study and respond to the other comments later.
|
So what is the preferred procedure here? I make the proposed changes in my replies to your comments, if you like them, and then make a new pull request? Also, you did not comment on my alternative suggestion to merge the telnet_translate_eol() functionality into _process() and have it be enabled by a bit in the flags byte for telnet_init(). I consider that a much cleaner solution, but I wrote it initially as a separate function so as not to disturb any existing functionality. |
Includes fixing a bug in telnet_translate_eol() for the case where there are multiple CRLF instances in one buffer.
|
Yes, I definitely prefer PR #34 If you are OK with the changes I have made to the options flag byte. As we both have said, it is cleaner for the user. |
|
@seanmiddleditch - Is there anything else you need from me for this? Any discussion you want from me or someone else? |
|
Nope, I'm re-reviewing the other change now. I'm going to close this out in favor of the other approach. Thanks. :) |
As you offered, here is a pull request to add functions to perform the EOL translation.
I made the assumption here that it was OK to add some internal flags in the state tracker flags byte to hold the current state of BINARY transmission and reception modes and to set those flags in _set_rfc1143.
I believe I have followed the style and documentation conventions correctly.